Re: [RFC PATCH 1/3] vfio: selftests: Allow run.sh to bind to more than one device

[David Matlack <dmatlack@google.com>]

On 2025-06-26 06:04 PM, Aaron Lewis wrote:
> Refactor the script "run.sh" to allow it to bind to more than one device
> at a time. Previously, the script would allow one BDF to be passed in as
> an argument to the script.  Extend this behavior to allow more than
> one, e.g.
> 
>   $ ./run.sh -d 0000:17:0c.1 -d 0000:17:0c.2 -d 0000:16:01.7 -s
> 
> This results in unbinding the devices 0000:17:0c.1, 0000:17:0c.2 and
> 0000:16:01.7 from their current drivers, binding them to the
> vfio-pci driver, then breaking out into a shell.
> 
> When testing is complete simply exit the shell to have those devices
> unbind from the vfio-pci driver and rebind to their previous ones.

Thanks for adding this. I also planning to upstream the change I just
made to our Google internal copy of the VFIO selftests to split this
script up into a setup and cleanup that will make it easier to use
multiple devices.

The setup script would let you run it multiple times to set up multiple
devices:

  tools/testing/seftests/vfio/setup.sh -d BDF_1
  tools/testing/seftests/vfio/setup.sh -d BDF_2
  tools/testing/seftests/vfio/setup.sh -d BDF_3
  tools/testing/seftests/vfio/setup.sh -d BDF_4

State about in-use devices are stored on the filesystem so that run.sh
can find it.

Then all devices can later be cleaned up all at once (or individually):

  tools/testing/seftests/vfio/cleanup.sh -d BDF_1
  tools/testing/seftests/vfio/cleanup.sh

Would you be interested in picking up that change as part of this
series?

> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  tools/testing/selftests/vfio/run.sh | 73 +++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/testing/selftests/vfio/run.sh b/tools/testing/selftests/vfio/run.sh
> index 0476b6d7adc3..334934dab5c5 100755
> --- a/tools/testing/selftests/vfio/run.sh
> +++ b/tools/testing/selftests/vfio/run.sh
> @@ -2,11 +2,11 @@
>  
>  # Global variables initialized in main() and then used during cleanup() when
>  # the script exits.
> -declare DEVICE_BDF
> -declare NEW_DRIVER
> -declare OLD_DRIVER
> -declare OLD_NUMVFS
> -declare DRIVER_OVERRIDE
> +declare -a DEVICE_BDFS
> +declare -a NEW_DRIVERS
> +declare -a OLD_DRIVERS
> +declare -a OLD_NUMVFS
> +declare -a DRIVER_OVERRIDES
>  
>  function write_to() {
>  	# Unfortunately set -x does not show redirects so use echo to manually
> @@ -36,10 +36,20 @@ function clear_driver_override() {
>  }
>  
>  function cleanup() {
> -	if [ "${NEW_DRIVER}"      ]; then unbind ${DEVICE_BDF} ${NEW_DRIVER} ; fi
> -	if [ "${DRIVER_OVERRIDE}" ]; then clear_driver_override ${DEVICE_BDF} ; fi
> -	if [ "${OLD_DRIVER}"      ]; then bind ${DEVICE_BDF} ${OLD_DRIVER} ; fi
> -	if [ "${OLD_NUMVFS}"      ]; then set_sriov_numvfs ${DEVICE_BDF} ${OLD_NUMVFS} ; fi
> +	for i in "${!DEVICE_BDFS[@]}"; do
> +		if [ ${NEW_DRIVERS[$i]} != false      ]; then unbind ${DEVICE_BDFS[$i]} ${NEW_DRIVERS[$i]}; fi
> +		if [ ${DRIVER_OVERRIDES[$i]} != false ]; then clear_driver_override ${DEVICE_BDFS[$i]}; fi
> +		if [ ${OLD_DRIVERS[$i]} != false      ]; then bind ${DEVICE_BDFS[$i]} ${OLD_DRIVERS[$i]}; fi
> +		if [ ${OLD_NUMVFS[$i]} != false       ]; then set_sriov_numvfs ${DEVICE_BDFS[$i]} ${OLD_NUMVFS[$i]}; fi
> +	done

If you want false to work here then you need to preinitialize all of the
arrays with false. Otherwise if an error occurs part-way through the
loop in main then these arrays will be partially initialized.

I think it would be simpler to just continue using the empty string
to indicate "false" since that will also work for uninitialized arrays.

> +}
> +
> +function get_bdfs_string() {
> +	local bdfs_str;
> +
> +	printf -v bdfs_str '%s,' "${DEVICE_BDFS[@]}"
> +	bdfs_str="${bdfs_str%,}"
> +	echo "${bdfs_str}"
>  }
>  
>  function usage() {
> @@ -60,7 +70,7 @@ function main() {
>  
>  	while getopts "d:hs" opt; do
>  		case $opt in
> -			d) DEVICE_BDF="$OPTARG" ;;
> +			d) DEVICE_BDFS+=("$OPTARG") ;;
>  			s) shell=true ;;
>  			*) usage ;;
>  		esac
> @@ -73,33 +83,44 @@ function main() {
>  	[ ! "${shell}" ] && [ $# = 0 ] && usage
>  
>  	# Check that the user passed in a BDF.
> -	[ "${DEVICE_BDF}" ] || usage
> +	[[ -n "${DEVICE_BDFS[@]}" ]] || usage
>  
>  	trap cleanup EXIT
>  	set -e
>  
> -	test -d /sys/bus/pci/devices/${DEVICE_BDF}
> +	for device_bdf in "${DEVICE_BDFS[@]}"; do
> +		local old_numvf=false
> +		local old_driver=false
> +		local driver_override=false

If an error happens between here and where the arrays are updated below
then cleanup() won't unwind the change.

Can you initialize the arrays here instead of local variables? That will
fix it.

>  
> -	if [ -f /sys/bus/pci/devices/${DEVICE_BDF}/sriov_numvfs ]; then
> -		OLD_NUMVFS=$(cat /sys/bus/pci/devices/${DEVICE_BDF}/sriov_numvfs)
> -		set_sriov_numvfs ${DEVICE_BDF} 0
> -	fi
> +		test -d /sys/bus/pci/devices/${device_bdf}
>  
> -	if [ -L /sys/bus/pci/devices/${DEVICE_BDF}/driver ]; then
> -		OLD_DRIVER=$(basename $(readlink -m /sys/bus/pci/devices/${DEVICE_BDF}/driver))
> -		unbind ${DEVICE_BDF} ${OLD_DRIVER}
> -	fi
> +		if [ -f /sys/bus/pci/devices/${device_bdf}/sriov_numvfs ]; then
> +			old_numvf=$(cat /sys/bus/pci/devices/${device_bdf}/sriov_numvfs)
> +			set_sriov_numvfs ${device_bdf} 0
> +		fi
> +
> +		if [ -L /sys/bus/pci/devices/${device_bdf}/driver ]; then
> +			old_driver=$(basename $(readlink -m /sys/bus/pci/devices/${device_bdf}/driver))
> +			unbind ${device_bdf} ${old_driver}
> +		fi
>  
> -	set_driver_override ${DEVICE_BDF} vfio-pci
> -	DRIVER_OVERRIDE=true
> +		set_driver_override ${device_bdf} vfio-pci
> +		driver_override=true
>  
> -	bind ${DEVICE_BDF} vfio-pci
> -	NEW_DRIVER=vfio-pci
> +		bind ${device_bdf} vfio-pci
> +
> +		NEW_DRIVERS+=(vfio-pci)
> +		OLD_DRIVERS+=(${old_driver})
> +		OLD_NUMVFS+=(${old_numvf})
> +		DRIVER_OVERRIDES+=(${driver_override})
> +	done
>  
>  	echo
>  	if [ "${shell}" ]; then
> -		echo "Dropping into ${SHELL} with VFIO_SELFTESTS_BDF=${DEVICE_BDF}"
> -		VFIO_SELFTESTS_BDF=${DEVICE_BDF} ${SHELL}
> +		local bdfs_str=$(get_bdfs_string);
> +		echo "Dropping into ${SHELL} with VFIO_SELFTESTS_BDFS=${bdfs_str}"
> +		VFIO_SELFTESTS_BDFS=${bdfs_str} ${SHELL}

What's the reason to use get_bdfs_string() instead of just:

  VFIO_SELFTESTS_BDFS="${DEVICE_BDFS[@]}" ${SHELL}

?

>  	else
>  		"$@" ${DEVICE_BDF}

This should be:

		"$@" ${DEVICE_BDFS[@]}

>  	fi
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
>