Re: [PATCH 01/12] vfio: selftests: Split run.sh into separate scripts

[David Matlack <dmatlack@google.com>]

On 2025-11-10 08:51 AM, Raghavendra Rao Ananta wrote:
> On Thu, Oct 9, 2025 at 4:56 AM David Matlack <dmatlack@google.com> wrote:
> >
> > If the future, VFIO selftests can automatically detect set up devices by
> nit: s/If/In

Ack

> > +       rm -rf ${DEVICES_DIR}
> Since the cleanup.sh can potentially be run against a single device,
> we should probably check if no devices exist under ${DEVICES_DIR}
> before removing the entire dir.

Oof, good catch. Will fix in v2.

> > +function bind() {
> > +       write_to /sys/bus/pci/drivers/${2}/bind ${1}
> > +}
> Since these scripts reside within the selftests/vfio/ dir, and most of
> these functions target PCI aspects, should we hardcode `vfio-pci` as
> the driver instead of passing the arg around? This is a general
> question/suggestion for the patch. We can even rename it to
> <op>_vfio_pci() (bind_vfio_pci() for example) for better clarity.

This function is also used to bind the device back to whatever driver it
was originally bound to in cleanup.sh. So "${2}" is not always
"vfio-pci".

> > +function set_sriov_numvfs() {
> > +       write_to /sys/bus/pci/devices/${1}/sriov_numvfs ${2}
> > +}
> > +
> Similar to get_sriov_numvfs(), shall we check the existence of the
> 'sriov_numvfs' first? In the current workflow, we indirectly check for
> the file using get_sriov_numvfs() or look for 'sriov_numvfs' in the
> ${device_bdf}. However, an independent check would be cleaner and
> safer (for any future references). WDYT?

I don't think set_sriov_numvfs() should silently do nothing if
sriov_numvfs file does not exist. Calling set_sriov_numvfs() on a device
that does not support SR-IOV indicates a bug in the caller, so it should
fail IMO.

> > +               driver=$(get_driver ${device_bdf})
> > +               if [ "${driver}" ]; then
> > +                       unbind ${device_bdf} ${driver}
> > +                       echo ${driver} > ${device_dir}/driver
> Sorry, what is the purpose of writing the driver's name to the
> "driver" file? Isn't "unbind" sufficient?

So that cleanup.sh can bind the device back to this driver. The goal is
that cleanup.sh returns the device back to the state it was in before
setup.sh. i.e. cleanup.sh needs to undo everything done by setup.sh.

> > +               fi
> > +
> Since vfio-pci can be built as a module that may not be loaded yet (or
> even disabled), do you think it's worth checking before binding the
> device to the driver? Of course, these operations would fail, but
> would it be better if we informed users why?

If that were to happen, I think it will be debuggable by the user.

write_to() in lib.sh always prints out what setup.sh and cleanup.sh are
trying to do. So the user will see something like:

  echo BDF > /sys/bus/pci/drivers/vfio-pci/bind
  Error: No such file or directory

And then the user can go investigate why /sys/bus/pci/drivers/ does not
exist. I think if we try to make the script diagnose why something
failed, that will be an endless game of whack-a-mole and the scripts
will start getting long and complex.