On Wed, Apr 30, 2025 at 6:06 AM Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> On Mon, Apr 28, 2025 at 04:48:11PM -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 automatically built, though not used.
> >
> >Only tested on x86.
> >
> >To run:
> >
> > $ tools/testing/selftests/vsock/vmtest.sh
>
> I tried and it's working, but I have a lot of these messages in the
> output:
> dmesg: read kernel buffer failed: Operation not permitted
>
> I'm on Fedora 41:
>
> $ uname -r
> 6.14.4-200.fc41.x86_64
>
> >
> >or
> >
> > $ make -C tools/testing/selftests TARGETS=vsock run_tests
> >
> >Results:
> > # linux/tools/testing/selftests/vsock/vmtest.log
> > setup: Building kernel and tests
> > setup: Booting up VM
> > setup: VM booted up
> > test:vm_server_host_client:guest: Control socket listening on
> > 0.0.0.0:51000
> > test:vm_server_host_client:guest: Control socket connection
> > accepted...
> > [...]
> > test:vm_loopback:guest: 30 - SOCK_STREAM retry failed connect()...ok
> > test:vm_loopback:guest: 31 - SOCK_STREAM SO_LINGER null-ptr-deref...ok
> > test:vm_loopback:guest: 31 - SOCK_STREAM SO_LINGER null-ptr-deref...ok
> >
> >Future work can include vsock_diag_test.
> >
> >vmtest.sh is loosely based off of tools/testing/selftests/net/pmtu.sh,
> >which was picked out of the bag of tests I knew to work with NIPA.
> >
> >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). This aspect is worth review, as I'm not aware of all of the
> >enviroments where this would run.
>
> Yeah, an opinion from net maintainers on this would be great.
>
> >
> >Signed-off-by: Bobby Eshleman <bobbyeshle...@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-3901a2733...@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-f35a81dab...@gmail.com
> >---
> > MAINTAINERS | 1 +
> > tools/testing/selftests/vsock/.gitignore | 1 +
> > tools/testing/selftests/vsock/Makefile | 9 +
> > tools/testing/selftests/vsock/config.vsock | 10 +
> > tools/testing/selftests/vsock/settings | 1 +
> > tools/testing/selftests/vsock/vmtest.sh | 354
> > +++++++++++++++++++++++++++++
> > 6 files changed, 376 insertions(+)
>
> While applying git warned about trailing spaces, but also checkpatch is
> warning about them, please can you fix at least the errors (and maybe
> the misspelling):
>
> $ ./scripts/checkpatch.pl --strict -g master..HEAD --codespell
> WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit
> description?)
> #31:
> test:vm_server_host_client:guest: Control socket listening on
> 0.0.0.0:51000
>
> WARNING: 'enviroments' may be misspelled - perhaps 'environments'?
> #48:
> enviroments where this would run.
> ^^^^^^^^^^^
>
> ERROR: trailing whitespace
> #174: FILE: tools/testing/selftests/vsock/vmtest.sh:47:
> +^Ivm_server_host_client^IRun vsock_test in server mode on the VM and in
> client mode on the host.^I$
>
> WARNING: line length of 104 exceeds 100 columns
> #174: FILE: tools/testing/selftests/vsock/vmtest.sh:47:
> + vm_server_host_client Run vsock_test in server mode on the VM and
> in client mode on the host.
>
> ERROR: trailing whitespace
> #175: FILE: tools/testing/selftests/vsock/vmtest.sh:48:
> +^Ivm_client_host_server^IRun vsock_test in client mode on the VM and in
> server mode on the host.^I$
>
> WARNING: line length of 104 exceeds 100 columns
> #175: FILE: tools/testing/selftests/vsock/vmtest.sh:48:
> + vm_client_host_server Run vsock_test in client mode on the VM and
> in server mode on the host.
>
> ERROR: trailing whitespace
> #176: FILE: tools/testing/selftests/vsock/vmtest.sh:49:
> +^Ivm_loopback^I^IRun vsock_test using the loopback transport in the VM.^I$
>
> ERROR: trailing whitespace
> #443: FILE: tools/testing/selftests/vsock/vmtest.sh:316:
> +IFS="^I$
>
> total: 4 errors, 4 warnings, 0 checks, 382 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
> mechanically convert to the typical style using --fix or --fix-inplace.
>
> NOTE: Whitespace errors detected.
> You may wish to use scripts/cleanpatch or scripts/cleanfile
>
> Commit ffa17d673263 ("selftests/vsock: add initial vmtest.sh for vsock") has
> style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
This might not have been the right choice, but I adopted the technique
from the pmtu selftest of
using a tab at the end of the help text lines and then splitting with
"IFS=\t\n". I guess the win is that
running the script has the side effect of checking that the help text
has been updated, and the tab
is not visible?
I think we could do something similar with bash arrays though and have
no checkpatch complaints, and
not having to do "git commit --no-verify" to bypass the checks.
> >
> >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..1950aa8ac68c0831c12c1aaa429da45bbe41e60f
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/.gitignore
> >@@ -0,0 +1 @@
> >+vsock_selftests.log
> >diff --git a/tools/testing/selftests/vsock/Makefile
> >b/tools/testing/selftests/vsock/Makefile
> >new file mode 100644
> >index
> >0000000000000000000000000000000000000000..6fded8c4d593541a6f7462147bffcb719def378f
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/Makefile
> >@@ -0,0 +1,9 @@
> >+# SPDX-License-Identifier: GPL-2.0
> >+.PHONY: all
> >+all:
> >+
> >+TEST_PROGS := vmtest.sh
> >+EXTRA_CLEAN := vmtest.log
> >+
> >+include ../lib.mk
> >+
> >diff --git a/tools/testing/selftests/vsock/config.vsock
> >b/tools/testing/selftests/vsock/config.vsock
> >new file mode 100644
> >index
> >0000000000000000000000000000000000000000..9e0fb2270e6a2fc0beb5f0d9f0bc37158d0a9d23
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/config.vsock
> >@@ -0,0 +1,10 @@
> >+CONFIG_VSOCKETS=y
> >+CONFIG_VSOCKETS_DIAG=y
> >+CONFIG_VSOCKETS_LOOPBACK=y
> >+CONFIG_VMWARE_VMCI_VSOCKETS=y
> >+CONFIG_VIRTIO_VSOCKETS=y
> >+CONFIG_VIRTIO_VSOCKETS_COMMON=y
> >+CONFIG_HYPERV_VSOCKETS=y
> >+CONFIG_VMWARE_VMCI=y
> >+CONFIG_VHOST_VSOCK=y
> >+CONFIG_HYPERV=y
> >diff --git a/tools/testing/selftests/vsock/settings
> >b/tools/testing/selftests/vsock/settings
> >new file mode 100644
> >index
> >0000000000000000000000000000000000000000..e7b9417537fbc4626153b72e8f295ab4594c844b
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/settings
> >@@ -0,0 +1 @@
> >+timeout=0
> >diff --git a/tools/testing/selftests/vsock/vmtest.sh
> >b/tools/testing/selftests/vsock/vmtest.sh
> >new file mode 100755
> >index
> >0000000000000000000000000000000000000000..d70b9446e531d6d20beb24ddeda2cf0a9f7e9a39
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/vmtest.sh
> >@@ -0,0 +1,354 @@
> >+#!/bin/bash
> >+# SPDX-License-Identifier: GPL-2.0
> >+#
> >+# Copyright (c) 2025 Meta Platforms, Inc. and affiliates
> >+#
> >+# Dependencies:
> >+# * virtme-ng
> >+# * busybox-static (used by virtme-ng)
> >+# * qemu (used by virtme-ng)
> >+
> >+SCRIPT_DIR="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
> >+KERNEL_CHECKOUT=$(realpath ${SCRIPT_DIR}/../../../..)
> >+QEMU=$(command -v qemu-system-$(uname -m))
> >+VERBOSE=0
> >+SKIP_BUILD=0
> >+VSOCK_TEST=${KERNEL_CHECKOUT}/tools/testing/vsock/vsock_test
> >+
> >+TEST_GUEST_PORT=51000
> >+TEST_HOST_PORT=50000
> >+TEST_HOST_PORT_LISTENER=50001
> >+SSH_GUEST_PORT=22
> >+SSH_HOST_PORT=2222
> >+VSOCK_CID=1234
> >+WAIT_PERIOD=3
> >+WAIT_PERIOD_MAX=20
> >+
> >+QEMU_PIDFILE=/tmp/qemu.pid
> >+
> >+# 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.
> >+QEMU_OPTS=""
> >+QEMU_OPTS="${QEMU_OPTS} -netdev
> >user,id=n0,hostfwd=tcp::${TEST_HOST_PORT}-:${TEST_GUEST_PORT}"
> >+QEMU_OPTS="${QEMU_OPTS},hostfwd=tcp::${SSH_HOST_PORT}-:${SSH_GUEST_PORT}"
> >+QEMU_OPTS="${QEMU_OPTS} -device virtio-net-pci,netdev=n0"
> >+QEMU_OPTS="${QEMU_OPTS} -device vhost-vsock-pci,guest-cid=${VSOCK_CID}"
> >+QEMU_OPTS="${QEMU_OPTS} --pidfile ${QEMU_PIDFILE}"
>
> What about:
>
> QEMU_OPTS=" \
> -netdev
> user,id=n0,hostfwd=tcp::${TEST_HOST_PORT}-:${TEST_GUEST_PORT},hostfwd=tcp::${SSH_HOST_PORT}-:${SSH_GUEST_PORT}
> \
> -device virtio-net-pci,netdev=n0 \
> -device vhost-vsock-pci,guest-cid=${VSOCK_CID} \
> --pidfile ${QEMU_PIDFILE}
> "
>
> Not a strong opinion, just I see too much "QEMU_OPTS="${QEMU_OPTS}" xD
> You can leave it in that way if you prefer.
>
That's totally fine by me, no strong opinion on my side. I tend to do
the string-building thing
across the board because for some reason multiline strings look odd to
me when inside
function scope, and string-building looks like the same pattern
everywhere. Definitely not
a preference I hold tightly though.
> >+KERNEL_CMDLINE="virtme.dhcp net.ifnames=0 biosdevname=0 virtme.ssh
> >virtme_ssh_user=$USER"
> >+
> >+LOG=${SCRIPT_DIR}/vmtest.log
> >+
> >+# Name Description
> >+avail_tests="
> >+ vm_server_host_client Run vsock_test in server mode on the VM and
> >in client mode on the host.
> >+ vm_client_host_server Run vsock_test in client mode on the VM and
> >in server mode on the host.
> >+ vm_loopback Run vsock_test using the loopback transport
> >in the VM.
> >+"
> >+
> >+usage() {
> >+ echo
> >+ echo "$0 [OPTIONS] [TEST]..."
> >+ echo "If no TEST argument is given, all tests will be run."
> >+ echo
> >+ echo "Options"
> >+ echo " -v: verbose output"
> >+ echo " -s: skip build"
> >+ echo
> >+ echo "Available tests${avail_tests}"
> >+ exit 1
> >+}
> >+
> >+die() {
> >+ echo "$*" >&2
> >+ exit 1
> >+}
> >+
> >+vm_ssh() {
> >+ ssh -q -o UserKnownHostsFile=/dev/null -p 2222 localhost $*
>
> ${SSH_HOST_PORT} instead of 2222, right?
>
Oops. Yep, will fix.
> The rest LGTM!
>
> Thanks,
> Stefano
>
Thanks for the review!
Best,
Bobby
