Message-ID: <tabqpll3r76jhx2ujayry25a7ujfsikm7tejv5sviayyojlmwz@amcchbc5xiqp>
Date: Tue, 27 May 2025 17:55:13 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Bobby Eshleman <bobbyeshleman@...il.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	kvm@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org, 
	virtualization@...ts.linux.dev, netdev@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v8] selftests/vsock: add initial vmtest.sh for
 vsock

On Tue, May 27, 2025 at 07:30:29AM -0700, Bobby Eshleman wrote:
>On Mon, May 26, 2025 at 01:18:18PM +0200, Stefano Garzarella wrote:
>> On Thu, May 22, 2025 at 09:59:07PM -0700, Bobby Eshleman wrote:
>> > This commit introduces a new vmtest.sh runner for vsock.
>> >
>> > It uses virtme-ng/qemu to run tests in a VM. The tests validate G2H,
>> > H2G, and loopback. The testing tools from tools/testing/vsock/ are
>> > reused. Currently, only vsock_test is used.
>> >
>> > VMCI and hyperv support is included in the config file to be built with
>> > the -b option, though not used in the tests.
>> >
>> > Only tested on x86.
>> >
>> > To run:
>> >
>> >  $ make -C tools/testing/selftests TARGETS=vsock
>> >  $ tools/testing/selftests/vsock/vmtest.sh
>> >
>> > or
>> >
>> >  $ make -C tools/testing/selftests TARGETS=vsock run_tests
>> >
>> > Example runs (after make -C tools/testing/selftests TARGETS=vsock):
>> >
>> > $ ./tools/testing/selftests/vsock/vmtest.sh
>> > 1..3
>> > ok 0 vm_server_host_client
>> > ok 1 vm_client_host_server
>> > ok 2 vm_loopback
>> > SUMMARY: PASS=3 SKIP=0 FAIL=0
>> > Log: /tmp/vsock_vmtest_m7DI.log
>> >
>> > $ ./tools/testing/selftests/vsock/vmtest.sh vm_loopback
>> > 1..1
>> > ok 0 vm_loopback
>> > SUMMARY: PASS=1 SKIP=0 FAIL=0
>> > Log: /tmp/vsock_vmtest_a1IO.log
>> >
>> > $ mkdir -p ~/scratch
>> > $ make -C tools/testing/selftests install TARGETS=vsock INSTALL_PATH=~/scratch
>> > [... omitted ...]
>> > $ cd ~/scratch
>> > $ ./run_kselftest.sh
>> > TAP version 13
>> > 1..1
>> > # timeout set to 300
>> > # selftests: vsock: vmtest.sh
>> > # 1..3
>> > # ok 0 vm_server_host_client
>> > # ok 1 vm_client_host_server
>> > # ok 2 vm_loopback
>> > # SUMMARY: PASS=3 SKIP=0 FAIL=0
>> > # Log: /tmp/vsock_vmtest_svEl.log
>> > ok 1 selftests: vsock: vmtest.sh
>> >
>> > Future work can include vsock_diag_test.
>> >
>> > Because vsock requires a VM to test anything other than loopback, this
>> > patch adds vmtest.sh as a kselftest itself. This is different than other
>> > systems that have a "vmtest.sh", where it is used as a utility script to
>> > spin up a VM to run the selftests as a guest (but isn't hooked into
>> > kselftest).
>> >
>> > Signed-off-by: Bobby Eshleman <bobbyeshleman@...il.com>
>> > ---
>> > Changes in v8:
>> > - remove NIPA comment from commit msg
>> > - remove tap_* functions and TAP_PREFIX
>> > - add -b for building kernel
>> > - Link to v7: https://lore.kernel.org/r/20250515-vsock-vmtest-v7-1-ba6fa86d6c2c@gmail.com
>> >
>> > Changes in v7:
>> > - fix exit code bug when ran is kselftest: use cnt_total instead of KSFT_NUM_TESTS
>> > - updated commit message with updated output
>> > - updated commit message with commands for installing/running as
>> >  kselftest
>> > - Link to v6: https://lore.kernel.org/r/20250515-vsock-vmtest-v6-1-9af1cc023900@gmail.com
>> >
>> > Changes in v6:
>> > - add make cmd in commit message in vmtest.sh example (Stefano)
>> > - check nonzero size of QEMU_PIDFILE using -s conditional (Stefano)
>> > - display log file path after tests so it is easier to find amongst other random names
>> > - cleanup qemu pidfile if qemu is unable to remove it
>> > - make oops/warning failures more obvious with 'FAIL' prefix in log
>> >  (simply saying 'detected' wasn't clear enough to identify failing
>> >  condition)
>> > - Link to v5: https://lore.kernel.org/r/20250513-vsock-vmtest-v5-1-4e75c4a45ceb@gmail.com
>> >
>> > Changes in v5:
>> > - make log file a tmpfile (Paolo)
>> > - make sure both default and user defined QEMU gets handled by the dependency check (Paolo)
>> > - increased VM boot up timeout from 1m to 3m for slow hosts (Paolo)
>> > - rename vm_setup -> vm_start (Paolo)
>> > - derive wait_for_listener from selftests/net/net_helper.sh to removes ss usage
>> > - Remove unused 'unset IFS' line (Paolo)
>> > - leave space after variable declarations (Paolo)
>> > - make QEMU_PIDFILE a tmp file (Paolo)
>> > - make everything readonly that is only read (Paolo)
>> > - source ktap_helpers.sh for KSFT_PASS and friends (Paolo)
>> > - don't check for timeout util (Paolo)
>> > - add missing usage string for -q qemu arg
>> > - add tap prefix to SUMMARY line since it isn't part of TAP protocol
>> > - exit with the correct status code based on failure/pass counts
>> > - Link to v4: https://lore.kernel.org/r/20250507-vsock-vmtest-v4-1-6e2a97262cd6@gmail.com
>> >
>> > Changes in v4:
>> > - do not use special tab delimiter for help string parsing (Stefano + Paolo)
>> > - fix paths for when installing kselftest and running out-of-tree (Paolo)
>> > - change vng to using running kernel instead of compiled kernel (Paolo)
>> > - use multi-line string for QEMU_OPTS (Stefano)
>> > - change timeout to 300s (Paolo)
>> > - skip if tools are not found and use kselftests status codes (Paolo)
>> > - remove build from vmtest.sh (Paolo)
>> > - change 2222 -> SSH_HOST_PORT (Stefano)
>> > - add tap-format output
>> > - add vmtest.log to gitignore
>> > - check for vsock_test binary and remind user to build it if missing
>> > - create a proper build in makefile
>> > - style fixes
>> > - add ssh, timeout, and pkill to dependency check, just in case
>> > - fix numerical comparison in conditionals
>> > - check qemu pidfile exists before proceeding (avoid wasting time waiting for ssh)
>> > - fix tracking of pass/fail bug
>> > - fix stderr redirection bug
>> > - Link to v3: https://lore.kernel.org/r/20250428-vsock-vmtest-v3-1-181af6163f3e@gmail.com
>> >
>> > Changes in v3:
>> > - use common conditional syntax for checking variables
>> > - use return value instead of global rc
>> > - fix typo TEST_HOST_LISTENER_PORT -> TEST_HOST_PORT_LISTENER
>> > - use SIGTERM instead of SIGKILL on cleanup
>> > - use peer-cid=1 for loopback
>> > - change sleep delay times into globals
>> > - fix test_vm_loopback logging
>> > - add test selection in arguments
>> > - make QEMU an argument
>> > - check that vng binary is on path
>> > - use QEMU variable
>> > - change <tab><backslash> to <space><backslash>
>> > - fix hardcoded file paths
>> > - add comment in commit msg about script that vmtest.sh was based off of
>> > - Add tools/testing/selftest/vsock/Makefile for kselftest
>> > - Link to v2: https://lore.kernel.org/r/20250417-vsock-vmtest-v2-1-3901a27331e8@gmail.com
>> >
>> > Changes in v2:
>> > - add kernel oops and warnings checker
>> > - change testname variable to use FUNCNAME
>> > - fix spacing in test_vm_server_host_client
>> > - add -s skip build option to vmtest.sh
>> > - add test_vm_loopback
>> > - pass port to vm_wait_for_listener
>> > - fix indentation in vmtest.sh
>> > - add vmci and hyperv to config
>> > - changed whitespace from tabs to spaces in help string
>> > - Link to v1: https://lore.kernel.org/r/20250410-vsock-vmtest-v1-1-f35a81dab98c@gmail.com
>> > ---
>> > MAINTAINERS                              |   1 +
>> > tools/testing/selftests/vsock/.gitignore |   2 +
>> > tools/testing/selftests/vsock/Makefile   |  16 ++
>> > tools/testing/selftests/vsock/config     | 114 ++++++++
>> > tools/testing/selftests/vsock/settings   |   1 +
>> > tools/testing/selftests/vsock/vmtest.sh  | 460 +++++++++++++++++++++++++++++++
>> > 6 files changed, 594 insertions(+)
>> >
>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index 657a67f9031ef7798c19ac63e6383d4cb18a9e1f..3fbdd7bbfce7196a3cc7db70203317c6bd0e51fd 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -25751,6 +25751,7 @@ F:	include/uapi/linux/vm_sockets.h
>> > F:	include/uapi/linux/vm_sockets_diag.h
>> > F:	include/uapi/linux/vsockmon.h
>> > F:	net/vmw_vsock/
>> > +F:	tools/testing/selftests/vsock/
>> > F:	tools/testing/vsock/
>> >
>> > VMALLOC
>> > diff --git a/tools/testing/selftests/vsock/.gitignore b/tools/testing/selftests/vsock/.gitignore
>> > new file mode 100644
>> > index 0000000000000000000000000000000000000000..9c5bf379480f829a14713d5f5dc7d525bc272e84
>> > --- /dev/null
>> > +++ b/tools/testing/selftests/vsock/.gitignore
>> > @@ -0,0 +1,2 @@
>> > +vmtest.log
>> > +vsock_test
>> > diff --git a/tools/testing/selftests/vsock/Makefile b/tools/testing/selftests/vsock/Makefile
>> > new file mode 100644
>> > index 0000000000000000000000000000000000000000..7ab4970e5e8a019be33f96a36f95c00573d7bfcf
>> > --- /dev/null
>> > +++ b/tools/testing/selftests/vsock/Makefile
>> > @@ -0,0 +1,16 @@
>> > +# SPDX-License-Identifier: GPL-2.0
>> > +
>> > +CURDIR := $(abspath .)
>> > +TOOLSDIR := $(abspath ../../..)
>> > +
>> > +$(OUTPUT)/vsock_test: $(TOOLSDIR)/testing/vsock/vsock_test
>> > +	install -m 755 $< $@
>> > +
>> > +$(TOOLSDIR)/testing/vsock/vsock_test:
>> > +	$(MAKE) -C $(TOOLSDIR)/testing/vsock vsock_test
>> > +
>> > +TEST_PROGS += vmtest.sh
>> > +TEST_GEN_FILES := vsock_test
>> > +
>> > +include ../lib.mk
>> > +
>>
>> I had modified the tests, but I noticed that they were not copied,
>> in fact we have no dependencies on the test sources.
>>
>> This may not be a problem in the selftest environment, but locally I
>> think it is.
>>
>> We can either use .PHONY and call `$(MAKE) -C ...` each time, or do
>> something like this (which solved my problem):
>>
>> diff --git a/tools/testing/selftests/vsock/Makefile b/tools/testing/selftests/vsock/Makefile
>> index 7ab4970e5e8a..d1bb1f63a9d1 100644
>> --- a/tools/testing/selftests/vsock/Makefile
>> +++ b/tools/testing/selftests/vsock/Makefile
>> @@ -3,11 +3,14 @@
>>  CURDIR := $(abspath .)
>>  TOOLSDIR := $(abspath ../../..)
>> -$(OUTPUT)/vsock_test: $(TOOLSDIR)/testing/vsock/vsock_test
>> +VSOCK_TEST_DIR := $(TOOLSDIR)/testing/vsock
>> +VSOCK_TEST_SRCS := $(wildcard $(VSOCK_TEST_DIR)/*.c $(VSOCK_TEST_DIR)/*.h)
>> +
>> +$(OUTPUT)/vsock_test: $(VSOCK_TEST_DIR)/vsock_test
>>         install -m 755 $< $@
>> -$(TOOLSDIR)/testing/vsock/vsock_test:
>> -       $(MAKE) -C $(TOOLSDIR)/testing/vsock vsock_test
>> +$(VSOCK_TEST_DIR)/vsock_test: $(VSOCK_TEST_SRCS)
>> +       $(MAKE) -C $(VSOCK_TEST_DIR) vsock_test
>>  TEST_PROGS += vmtest.sh
>>  TEST_GEN_FILES := vsock_test
>>
>>
>
>Makes sense!
>
>> > +
>> > +# virtme-ng offers a netdev for ssh when using "--ssh", but we also need a
>> > +# control port forwarded for vsock_test.  Because virtme-ng doesn't support
>> > +# adding an additional port to forward to the device created from "--ssh" and
>> > +# virtme-init mistakenly sets identical IPs to the ssh device and additional
>> > +# devices, we instead opt out of using --ssh, add the device manually, and also
>> > +# add the kernel cmdline options that virtme-init uses to setup the interface.
>>
>> This version of the script doesn't work for me, the VM starts, but I
>> can't connect in ssh. After spending some time debugging, I saw that
>> `sshd` was not being launched inside the VM.
>>
>> I think it depends on the version of vng (maybe recently updated on my
>> Fedora 41); here I have:
>> $ vng --version
>> virtme-ng 1.36
>>
>> Playing around with `vng`, I found that it now also supports ssh on
>> vsock (I think it is now default). By forcing it to use tcp
>> (`vng ... --ssh --ssh-tcp`), it adds another parameter to my cmdline:
>> `virtme_ssh_channel=tcp`.
>>
>> And indeed this change fixed my issues:
>> --- a/tools/testing/selftests/vsock/vmtest.sh
>> +++ b/tools/testing/selftests/vsock/vmtest.sh
>> @@ -39,7 +39,7 @@ readonly QEMU_OPTS="\
>>          -device vhost-vsock-pci,guest-cid=${VSOCK_CID} \
>>          --pidfile ${QEMU_PIDFILE} \
>>  "
>> -readonly KERNEL_CMDLINE="virtme.dhcp net.ifnames=0 biosdevname=0 virtme.ssh virtme_ssh_user=$USER"
>> +readonly KERNEL_CMDLINE="virtme.dhcp net.ifnames=0 biosdevname=0 virtme.ssh virtme_ssh_channel=tcp virtme_ssh_user=$USER"
>>  readonly LOG=$(mktemp /tmp/vsock_vmtest_XXXX.log)
>>  readonly TEST_NAMES=(vm_server_host_client vm_client_host_server vm_loopback)
>>  readonly TEST_DESCS=(
>>
>> That said, I'm concerned about the weakness of assuming the cmdlines
>> supported by vng (which do not seem stable to me at this point).
>>
>> Should we check the version of vng and at least print a warning to say
>> that this script is not tested on that version?
>>
>> Thanks,
>> Stefano
>>
>
>I'm also not a big fan of assuming cmdlines and I think adding the
>version warning sounds good. I have been using 1.33. One thought I'm
>having is that I should probably submit a PR to vng to either fix the
>multi-NIC setup in virtme-ng or add a hostfwd option... then restrict
>our assumptions here to only prior versions.

Yes, that would be great, but anyway for now I would say let's go with 
this since we're supposed to support these current versions I guess, and 
this hack at the end I think is doable.

BTW, thanks again for this useful work!
Stefano

Powered by blists - more mailing lists