On Fri, 2021-03-05 at 12:04 +0100, Andrew Jones wrote:
> Hi Elena,
>
> I think KVM selftests[1] is perhaps a better test framework for this
> type of test, but I'm not opposed to teaching kvm-unit-tests how to
> do this too. I have some ideas as to how to do it more generally
> though. Please see comments below.
>
> (If you are interested in trying to do the testing with KVM
> selftests,
> and would like some suggestions as to how to get started, then feel
> free to mail me.)
>
> [1] Linux src: tools/testing/selftests/kvm
>
>
> On Mon, Mar 01, 2021 at 09:33:19PM +0300, Elena Afanasova wrote:
> > Signed-off-by: Elena Afanasova <eafanasova@xxxxxxxxx>
> > ---
> > scripts/common.bash | 12 +++++--
> > scripts/runtime.bash | 9 +++++
> > x86/Makefile.common | 5 ++-
> > x86/Makefile.x86_64 | 2 ++
> > x86/ioregionfd-test.c | 84
> > +++++++++++++++++++++++++++++++++++++++++++
> > x86/ioregionfd_pio.c | 24 +++++++++++++
> > x86/unittests.cfg | 7 ++++
> > 7 files changed, 140 insertions(+), 3 deletions(-)
> > create mode 100644 x86/ioregionfd-test.c
> > create mode 100644 x86/ioregionfd_pio.c
> >
> > diff --git a/scripts/common.bash b/scripts/common.bash
> > index 7b983f7..d9f8556 100644
> > --- a/scripts/common.bash
> > +++ b/scripts/common.bash
> > @@ -14,6 +14,8 @@ function for_each_unittest()
> > local accel
> > local timeout
> > local rematch
> > + local helper_cmd
> > + local fifo
>
> I'd prefer not to add 'fifo' to the unittests.cfg parameter list, as
> that's specific to this particular "helper_cmd". Also, while in this
> case the helper runs along side the test, a generic helper may need
> to do pre-test and post-test work. So, I think we should follow the
> migration test model and pass the QEMU command line (minus the new
> -device pc-testdev part) to the helper program where it then does
> pre-test stuff (in this case mkfifo), possibly augments the test's
> QEMU command line (in this case with the -device pc-testdev part),
> and runs the test (in parallel with ioregionfd-helper in this case),
> and then does any post-test work (remove the fifos).
>
> You can write a Bash script that does most of that and which also
> launches ioregionfd-helper (notice, I'm suggesting we rename that
> from iorergionfd-test, as it's not a test, it's a test helper).
>
> >
> > exec {fd}<"$unittests"
> >
> > @@ -21,7 +23,7 @@ function for_each_unittest()
> > if [[ "$line" =~ ^\[(.*)\]$ ]]; then
> > rematch=${BASH_REMATCH[1]}
> > if [ -n "${testname}" ]; then
> > - $(arch_cmd) "$cmd" "$testname"
> > "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel"
> > "$timeout"
> > + $(arch_cmd) "$cmd" "$testname"
> > "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel"
> > "$timeout" "$helper_cmd" "$fifo"
> > fi
> > testname=$rematch
> > smp=1
> > @@ -32,6 +34,8 @@ function for_each_unittest()
> > check=""
> > accel=""
> > timeout=""
> > + helper_cmd=""
> > + fifo=""
> > elif [[ $line =~ ^file\ *=\ *(.*)$ ]]; then
> > kernel=$TEST_DIR/${BASH_REMATCH[1]}
> > elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
> > @@ -48,10 +52,14 @@ function for_each_unittest()
> > accel=${BASH_REMATCH[1]}
> > elif [[ $line =~ ^timeout\ *=\ *(.*)$ ]]; then
> > timeout=${BASH_REMATCH[1]}
> > + elif [[ $line =~ ^helper_cmd\ *=\ *(.*)$ ]]; then
> > + helper_cmd=$TEST_DIR/${BASH_REMATCH[1]}
> > + elif [[ $line =~ ^fifo\ *=\ *(.*)$ ]]; then
> > + fifo=${BASH_REMATCH[1]}
> > fi
> > done
> > if [ -n "${testname}" ]; then
> > - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp"
> > "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> > + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp"
> > "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> > "$helper_cmd" "$fifo"
> > fi
> > exec {fd}<&-
> > }
> > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > index 132389c..ba94ee5 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -81,6 +81,8 @@ function run()
> > local check="${CHECK:-$7}"
> > local accel="$8"
> > local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the
> > default
> > + local helper_cmd="${10}"
> > + local fifo="${11}"
> >
> > if [ -z "$testname" ]; then
> > return
> > @@ -139,6 +141,11 @@ function run()
> > echo $cmdline
> > fi
> >
> > + if [ -n "${helper_cmd}" ] && [ -n "${fifo}" ]; then
> > + mkfifo $fifo
> > + $helper_cmd $fifo &
> > + fi
> > +
> > # extra_params in the config file may contain backticks that
> > need to be
> > # expanded, so use eval to start qemu. Use "> >(foo)" instead
> > of a pipe to
> > # preserve the exit status.
> > @@ -159,6 +166,8 @@ function run()
> > print_result "FAIL" $testname "$summary"
> > fi
> >
> > + [ -n "${fifo}" ] && rm -rf $fifo
> > +
> > return $ret
> > }
>
> With the suggestion I'm making I think all the changes above in
> runtime.bash
> can go away, as we can simply do the following instead
>
> diff --git a/x86/run b/x86/run
> index 8b2425f45640..fdd6121e37ad 100755
> --- a/x86/run
> +++ b/x86/run
> @@ -39,6 +39,6 @@ fi
>
> command="${qemu} --no-reboot -nodefaults $pc_testdev -vnc none
> -serial stdio $pci_testdev"
> command+=" -machine accel=$ACCEL -kernel"
> -command="$(timeout_cmd) $command"
> +command="$(timeout_cmd) $(helper_cmd) $command"
>
> run_qemu ${command} "$@"
>
>
> Assuming we have helper_cmd defined in x86/run some how.
>
> >
> > diff --git a/x86/Makefile.common b/x86/Makefile.common
> > index 55f7f28..a5cd1d2 100644
> > --- a/x86/Makefile.common
> > +++ b/x86/Makefile.common
> > @@ -82,6 +82,9 @@ $(TEST_DIR)/hyperv_stimer.elf:
> > $(TEST_DIR)/hyperv.o
> >
> > $(TEST_DIR)/hyperv_connections.elf: $(TEST_DIR)/hyperv.o
> >
> > +$(TEST_DIR)/ioregionfd-test:
> > + $(CC) -o $@ $(TEST_DIR)/ioregionfd-test.c
> > +
> > arch_clean:
> > - $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
> > + $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf
> > $(TEST_DIR)/ioregionfd-test \
> > $(TEST_DIR)/.*.d lib/x86/.*.d \
> > diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
> > index 8134952..1a7a6b1 100644
> > --- a/x86/Makefile.x86_64
> > +++ b/x86/Makefile.x86_64
> > @@ -24,6 +24,8 @@ tests += $(TEST_DIR)/vmware_backdoors.flat
> > tests += $(TEST_DIR)/rdpru.flat
> > tests += $(TEST_DIR)/pks.flat
> > tests += $(TEST_DIR)/pmu_lbr.flat
> > +tests += $(TEST_DIR)/ioregionfd_pio.flat
> > +tests += $(TEST_DIR)/ioregionfd-test
>
> Please write as
>
> tests += $(TEST_DIR)/ioregionfd_pio.flat $(TEST_DIR)/ioregionfd-test
>
> >
> > ifneq ($(fcf_protection_full),)
> > tests += $(TEST_DIR)/cet.flat
> > diff --git a/x86/ioregionfd-test.c b/x86/ioregionfd-test.c
> > new file mode 100644
> > index 0000000..5ea5e57
> > --- /dev/null
> > +++ b/x86/ioregionfd-test.c
> > @@ -0,0 +1,84 @@
> > +#include <linux/ioregion.h>
> > +#include <string.h>
> > +#include <poll.h>
> > +#include <stdarg.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +
> > +void err_exit(const char *fmt, ...)
> > +{
> > + va_list args;
> > +
> > + va_start(args, fmt);
> > + vfprintf(stderr, fmt, args);
> > + va_end(args);
> > + exit(EXIT_FAILURE);
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + struct ioregionfd_cmd cmd;
> > + struct ioregionfd_resp resp;
> > + struct pollfd pollfd;
> > + int read_fd, write_fd = -1;
> > + unsigned long cdata = 0;
> > + int ret;
> > +
> > + if (argc < 2)
> > + err_exit("Usage: %s <read_fifo> <write_fifo>\n",
> > argv[0]);
> > +
> > + read_fd = open(argv[1], O_RDONLY);
> > + if (read_fd < 0)
> > + err_exit("failed to open read fifo %s\n", argv[1]);
> > +
> > + if (argc == 3) {
> > + write_fd = open(argv[2], O_WRONLY);
> > + if (write_fd < 0) {
> > + close(read_fd);
> > + err_exit("failed to open write fifo %s\n",
> > argv[2]);
> > + }
> > + }
> > +
> > + pollfd.fd = read_fd;
> > + pollfd.events = POLLIN;
> > +
> > + for (;;) {
> > + ret = poll(&pollfd, 1, -1);
> > + if (ret < 0) {
> > + close(read_fd);
> > + if (write_fd > 0)
> > + close(write_fd);
> > + err_exit("poll\n");
> > + }
> > +
> > + if (pollfd.revents & POLLIN) {
> > + ret = read(read_fd, &cmd, sizeof(cmd));
> > +
> > + switch (cmd.cmd) {
> > + case IOREGIONFD_CMD_READ:
> > + memset(&resp, 0, sizeof(resp));
> > + memcpy(&resp.data, &cdata, 1 <<
> > cmd.size_exponent);
> > + ret = write(write_fd, &resp,
> > sizeof(resp));
> > + break;
> > + case IOREGIONFD_CMD_WRITE:
> > + memcpy(&cdata, &cmd.data, 1 <<
> > cmd.size_exponent);
> > + if (cmd.resp) {
> > + memset(&resp, 0, sizeof(resp));
> > + ret = write(write_fd, &resp,
> > sizeof(resp));
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + } else
> > + break;
> > + }
> > +
> > + close(read_fd);
> > + if (write_fd > 0)
> > + close(write_fd);
> > +
> > + return 0;
> > +}
> > diff --git a/x86/ioregionfd_pio.c b/x86/ioregionfd_pio.c
> > new file mode 100644
> > index 0000000..eaf8aad
> > --- /dev/null
> > +++ b/x86/ioregionfd_pio.c
> > @@ -0,0 +1,24 @@
> > +#include "x86/vm.h"
> > +
> > +int main(int ac, char **av)
> > +{
> > + u32 expected = 0x11223344;
> > + u32 val = 0;
> > + u32 write_addr = 0x1234;
>
> Getting this address from the command line would allow us to only
> hard code it in one place, unittests.cfg.
>
> > + u32 read_addr = 0x1238;
> > +
> > + outb(expected, write_addr);
> > + val = inb(read_addr);
> > + report(val == (u8)expected, "%x\n", val);
> > +
> > + outw(expected, write_addr);
> > + val = inw(read_addr);
> > + report(val == (u16)expected, "%x\n", val);
> > +
> > + outl(expected, write_addr);
> > + val = inl(read_addr);
> > + report(val == expected, "%x\n", val);
> > +
> > + return report_summary();
> > +}
> > +
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index 0698d15..8001808 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -389,3 +389,10 @@ file = cet.flat
> > arch = x86_64
> > smp = 2
> > extra_params = -enable-kvm -m 2048 -cpu host
> > +
> > +[ioregion-pio]
> > +file = ioregionfd_pio.flat
> > +helper_cmd = ioregionfd-test
> > +fifo = /tmp/test1 /tmp/test2
> > +extra_params = -device pc-
> > testdev,addr=0x1234,size=8,rfifo=/tmp/test2,wfifo=/tmp/test1,pio=tr
> > ue
>
> If we wrap the QEMU invocation in the helper_cmd, then we can use
> mktemp to
> create our fifo node names. We also can put the address into a
> variable
> that we both pass to the QEMU command line augmentation and to the
> unit
> test through the unit test's command line.
>
> Thanks,
> drew
>
Hi Andrew,
Thank you for the feedback and code review! Will rework.
> > +arch = x86_64
> > --
> > 2.25.1
> >
