[U-Boot] [PATCH V2 1/3] itest: make memory access work under sandbox

itest accesses memory, and hence must map/unmap it. Without doing so, it accesses invalid addresses and crashes.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org Reviewed-by: Simon Glass sjg@chromium.org --- v2: No change. --- common/cmd_itest.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/common/cmd_itest.c b/common/cmd_itest.c index 76af62b46ee2..596341c9635a 100644 --- a/common/cmd_itest.c +++ b/common/cmd_itest.c @@ -15,6 +15,9 @@ #include <common.h> #include <config.h> #include <command.h> +#include <mapmem.h> + +#include <asm/io.h>
#define EQ 0 #define NE 1 @@ -49,16 +52,24 @@ static const op_tbl_t op_table [] = { static long evalexp(char *s, int w) { long l = 0; - long *p; + unsigned long addr; + void *buf;
/* if the parameter starts with a * then assume is a pointer to the value we want */ if (s[0] == '*') { - p = (long *)simple_strtoul(&s[1], NULL, 16); + addr = simple_strtoul(&s[1], NULL, 16); + buf = map_physmem(addr, w, MAP_WRBACK); + if (!buf) { + puts("Failed to map physical memory\n"); + return 0; + } switch (w) { - case 1: return((long)(*(unsigned char *)p)); - case 2: return((long)(*(unsigned short *)p)); - case 4: return(*p); + case 1: l = (long)(*(unsigned char *)buf); + case 2: l = (long)(*(unsigned short *)buf); + case 4: l = (long)(*(unsigned long *)buf); } + unmap_physmem(buf, w); + return l; } else { l = simple_strtoul(s, NULL, 16); }

In my patch series to replace fs/fat with "ff.c", I enhanced ff.c to optimize file reading, so that reads of contiguous clusters are submitted to the IO device as a single read. This test attempts to torture-test edge-cases of that enhancement.
BTW, the only way I found to validate that this script actually does create non-contiguous files was to manually inspect the FAT bitmap in a hex dump of the FAT image. hdparm --fibmap doesn't work on loop-mounted filesystems. filefrag -v -e seems to lie about files being contiguous when they aren't.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org --- v2: * Check for missing pre-requisite binaries, and print an explicit error message if any are missing. * Create mount-point rather than assuming it exists. * Add a comment describing the script's operation. --- test/fs/fat-noncontig-test.sh | 113 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100755 test/fs/fat-noncontig-test.sh
diff --git a/test/fs/fat-noncontig-test.sh b/test/fs/fat-noncontig-test.sh new file mode 100755 index 000000000000..f153c97bbf05 --- /dev/null +++ b/test/fs/fat-noncontig-test.sh @@ -0,0 +1,113 @@ +#!/bin/bash + +# (C) Copyright 2015 Stephen Warren +# +# SPDX-License-Identifier: GPL-2.0+ + +# This script tests U-Boot's FAT filesystem code's ability to read non- +# contiguous files. + +# When porting the ff.c FAT parsing code into U-Boot, it was found that ff.c +# always reads files cluster-by-cluster, which results in poor performance. +# This was solved by adding a patch to ff.c to coalesce reads of adjacent +# clusters. Since this patch needed to correctly handle non-contiguous files, +# this test was written to validate that. +# +# To execute the test, simply run it from the U-Boot source root directory: +# +# cd u-boot +# ./test/fs/fat-noncontig-test.sh +# +# The test will create a FAT filesystem image, record the CRC of a randomly +# generated file in the image, build U-Boot sandbox, invoke U-Boot sandbox to +# read the file and validate that the CRCs match. Expected output is shown +# below. The important part of the log is the penultimate line that contains +# either "PASS" or "FAILURE". +# +# mkfs.fat 3.0.26 (2014-03-07) +# +# +# U-Boot 2015.10-rc4-00018-g4b22a3e5513f (Oct 03 2015 - 13:49:23 -0600) +# +# DRAM: 128 MiB +# Using default environment +# +# In: serial +# Out: lcd +# Err: lcd +# Net: No ethernet found. +# => host bind 0 sandbox/fat-noncontig.img +# => load host 0:0 1000 noncontig.img +# 33584964 bytes read in 18 ms (1.7 GiB/s) +# => crc32 1000 $filesize 0 +# crc32 for 00001000 ... 02008743 ==> 6a080523 +# => if itest.l *0 != 2305086a; then echo FAILURE; else echo PASS; fi +# PASS +# => reset +# +# All temporary files used by this script are created in ./sandbox to avoid +# polluting the source tree. test/fs/fs-test.sh also uses this directory for +# the same purpose. +# +# TODO: Integrate this (and many other corner-cases e.g. different types of +# FAT) with fs-test.sh so that a single script tests everything filesystem- +# related. + +odir=sandbox +img=${odir}/fat-noncontig.img +mnt=${odir}/mnt +fill=/dev/urandom +testfn=noncontig.img +mnttestfn=${mnt}/${testfn} +crcaddr=0 +loadaddr=1000 + +for prereq in fallocate mkfs.fat dd crc32; do + if [ ! -x "`which $prereq`" ]; then + echo "Missing $prereq binary. Exiting!" + exit 1 + fi +done + +make O=${odir} -s sandbox_defconfig && make O=${odir} -s -j8 + +mkdir -p ${mnt} +if [ ! -f ${img} ]; then + fallocate -l 40M ${img} + mkfs.fat ${img} + + sudo mount -o loop,uid=$(id -u) ${img} ${mnt} + + for ((sects=8; sects < 512; sects += 8)); do + fn=${mnt}/keep-${sects}.img + dd if=${fill} of=${fn} bs=512 count=${sects} >/dev/null 2>&1 + fn=${mnt}/remove-${sects}.img + dd if=${fill} of=${fn} bs=512 count=${sects} >/dev/null 2>&1 + done + + rm -f ${mnt}/remove-*.img + + # 511 deliberately to trigger a file size that's not a multiple of the + # sector size (ignoring sizes that are multiples of both). + dd if=${fill} of=${mnttestfn} bs=511 >/dev/null 2>&1 + + sudo umount ${mnt} +fi + +sudo mount -o ro,loop,uid=$(id -u) ${img} ${mnt} +crc=0x`crc32 ${mnttestfn}` +sudo umount ${mnt} + +crc=`printf %02x%02x%02x%02x \ + $((${crc} & 0xff)) \ + $(((${crc} >> 8) & 0xff)) \ + $(((${crc} >> 16) & 0xff)) \ + $((${crc} >> 24))` + +./sandbox/u-boot << EOF +host bind 0 ${img} +load host 0:0 ${loadaddr} ${testfn} +crc32 ${loadaddr} $filesize ${crcaddr} +if itest.l *${crcaddr} != ${crc}; then echo FAILURE; else echo PASS; fi +reset +EOF

On 3 October 2015 at 20:56, Stephen Warren swarren@wwwdotorg.org wrote:
In my patch series to replace fs/fat with "ff.c", I enhanced ff.c to optimize file reading, so that reads of contiguous clusters are submitted to the IO device as a single read. This test attempts to torture-test edge-cases of that enhancement.
BTW, the only way I found to validate that this script actually does create non-contiguous files was to manually inspect the FAT bitmap in a hex dump of the FAT image. hdparm --fibmap doesn't work on loop-mounted filesystems. filefrag -v -e seems to lie about files being contiguous when they aren't.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org
v2:
- Check for missing pre-requisite binaries, and print an explicit error message if any are missing.
- Create mount-point rather than assuming it exists.
- Add a comment describing the script's operation.
test/fs/fat-noncontig-test.sh | 113 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100755 test/fs/fat-noncontig-test.sh
Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org

On Sat, Oct 03, 2015 at 01:56:47PM -0600, Stephen Warren wrote:
In my patch series to replace fs/fat with "ff.c", I enhanced ff.c to optimize file reading, so that reads of contiguous clusters are submitted to the IO device as a single read. This test attempts to torture-test edge-cases of that enhancement.
BTW, the only way I found to validate that this script actually does create non-contiguous files was to manually inspect the FAT bitmap in a hex dump of the FAT image. hdparm --fibmap doesn't work on loop-mounted filesystems. filefrag -v -e seems to lie about files being contiguous when they aren't.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!

In the following snippet:
if [ ! -x `which $prereq` ]; then
When $prereq does not exist, `which $prereq` evaluates to the empty string, which results in *no* argument being passed to the -x operator, which then evaluates to true, which is the equivalent of the prereq having been found. In order for this to fail as expected, we must pass an empty argument, which then causes -x to fail. Do this by wrapping the `` in quotes so there's always an argument to -x, even if the value of the argument is zero-length.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org --- v2: New patch. --- test/fs/fs-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh index b88a67ca066d..a4200964355e 100755 --- a/test/fs/fs-test.sh +++ b/test/fs/fs-test.sh @@ -58,7 +58,7 @@ GB2p5="${MOUNT_DIR}/${BIG_FILE}" # Check if the prereq binaries exist, or exit function check_prereq() { for prereq in $PREREQ_BINS; do - if [ ! -x `which $prereq` ]; then + if [ ! -x "`which $prereq`" ]; then echo "Missing $prereq binary. Exiting!" exit fi

On 3 October 2015 at 20:56, Stephen Warren swarren@wwwdotorg.org wrote:
In the following snippet:
if [ ! -x `which $prereq` ]; then
When $prereq does not exist, `which $prereq` evaluates to the empty string, which results in *no* argument being passed to the -x operator, which then evaluates to true, which is the equivalent of the prereq having been found. In order for this to fail as expected, we must pass an empty argument, which then causes -x to fail. Do this by wrapping the `` in quotes so there's always an argument to -x, even if the value of the argument is zero-length.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org
v2: New patch.
test/fs/fs-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org

On Sat, Oct 03, 2015 at 01:56:48PM -0600, Stephen Warren wrote:
In the following snippet:
if [ ! -x `which $prereq` ]; then
When $prereq does not exist, `which $prereq` evaluates to the empty string, which results in *no* argument being passed to the -x operator, which then evaluates to true, which is the equivalent of the prereq having been found. In order for this to fail as expected, we must pass an empty argument, which then causes -x to fail. Do this by wrapping the `` in quotes so there's always an argument to -x, even if the value of the argument is zero-length.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!

On Sat, Oct 03, 2015 at 01:56:46PM -0600, Stephen Warren wrote:
itest accesses memory, and hence must map/unmap it. Without doing so, it accesses invalid addresses and crashes.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (3)
-
Simon Glass
-
Stephen Warren
-
Tom Rini