[RFC PATCH kvm-unit-tests] x86: add ioregionfd fast PIO test

[RFC PATCH kvm-unit-tests] x86: add ioregionfd fast PIO test

[Elena Afanasova]

Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
---
 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
 
 	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
 }
 
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
 
 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;
+	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=true
+arch = x86_64
-- 
2.25.1

Re: [RFC PATCH kvm-unit-tests] x86: add ioregionfd fast PIO test

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On Mon, Mar 01, 2021 at 09:33:19PM +0300, Elena Afanasova wrote:
> @@ -159,6 +166,8 @@ function run()
>          print_result "FAIL" $testname "$summary"
>      fi
>  
> +    [ -n "${fifo}" ] && rm -rf $fifo

Is there a guarantee that $helper_cmd has terminated? If not then it
would be good to store its pid and invoke kill $helper_cmd_pid here
(maybe with an error message indicating helper_cmd hung).

> 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 @@

Please add a comment describing the purpose of this program.

> +	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");
> +		}

Is poll() necessary? I think a blocking read(read_fd) would have the
same effect and simplify the code?

> 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 @@

Please add a comment explaining the purpose of this test.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH kvm-unit-tests] x86: add ioregionfd fast PIO test

[Andrew Jones]

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@gmail.com>
> ---
>  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=true

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

> +arch = x86_64
> -- 
> 2.25.1
> 

Re: [RFC PATCH kvm-unit-tests] x86: add ioregionfd fast PIO test

[Elena Afanasova]

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@gmail.com>
> > ---
> >  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
> >