RE: [PATCH v4 11/12] samples: add vfio-mdev-pci driver

Liu, Yi L
2020-01-16T13:23:40Z

> From: Cornelia Huck [mailto:cohuck@redhat.com]
> Sent: Wednesday, January 15, 2020 8:30 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [PATCH v4 11/12] samples: add vfio-mdev-pci driver
>
> On Tue, 7 Jan 2020 20:01:48 +0800
> Liu Yi L <yi.l.liu@intel.com> wrote:
>
> > This patch adds sample driver named vfio-mdev-pci. It is to wrap
> > a PCI device as a mediated device. For a pci device, once bound
> > to vfio-mdev-pci driver, user space access of this device will
> > go through vfio mdev framework. The usage of the device follows
> > mdev management method. e.g. user should create a mdev before
> > exposing the device to user-space.
> >
> > Benefit of this new driver would be acting as a sample driver
> > for recent changes from "vfio/mdev: IOMMU aware mediated device"
> > patchset. Also it could be a good experiment driver for future
> > device specific mdev migration support. This sample driver only
> > supports singleton iommu groups, for non-singleton iommu groups,
> > this sample driver doesn't work. It will fail when trying to assign
> > the non-singleton iommu group to VMs.
> >
> > To use this driver:
> > a) build and load vfio-mdev-pci.ko module
> > execute "make menuconfig" and config CONFIG_SAMPLE_VFIO_MDEV_PCI
> > then load it with following command:
> > > sudo modprobe vfio
> > > sudo modprobe vfio-pci
> > > sudo insmod samples/vfio-mdev-pci/vfio-mdev-pci.ko
> >
> > b) unbind original device driver
> > e.g. use following command to unbind its original driver
> > > echo $dev_bdf > /sys/bus/pci/devices/$dev_bdf/driver/unbind
> >
> > c) bind vfio-mdev-pci driver to the physical device
> > > echo $vend_id $dev_id > /sys/bus/pci/drivers/vfio-mdev-pci/new_id
> >
> > d) check the supported mdev instances
> > > ls /sys/bus/pci/devices/$dev_bdf/mdev_supported_types/
> > vfio-mdev-pci-type_name
> > > ls /sys/bus/pci/devices/$dev_bdf/mdev_supported_types/\
> > vfio-mdev-pci-type_name/
> > available_instances create device_api devices name
> >
> > e) create mdev on this physical device (only 1 instance)
> > > echo "83b8f4f2-509f-382f-3c1e-e6bfe0fa1003" > \
> > /sys/bus/pci/devices/$dev_bdf/mdev_supported_types/\
> > vfio-mdev-pci-type_name/create
> >
> > f) passthru the mdev to guest
> > add the following line in QEMU boot command
> > -device vfio-pci,\
> > sysfsdev=/sys/bus/mdev/devices/83b8f4f2-509f-382f-3c1e-e6bfe0fa1003
> >
> > g) destroy mdev
> > > echo 1 > /sys/bus/mdev/devices/83b8f4f2-509f-382f-3c1e-e6bfe0fa1003/\
> > remove
>
> I think much/most of those instructions should go (additionally) into
> the sample driver source.

yes, it would be helpful to add it in a doc.

> Otherwise, it's not clear to the reader why
> they should wrap the device in mdev instead of simply using a normal
> vfio-pci device.

Actually, the reason of wrapping device in mdev instead of simply using
a normal vfio-pci is to let vendor specific driver to intercept some
device access which is not allowed in vfio-pci usage. We only have PCI
config space access intercepted and some other special accesses intercepted
in vfio-pci. While for some vendor specific handling, it would be nice
to have a way to let vendor specific driver intercept in. mdev allows it.

And back to the purpose of introducing this sample driver, it is supposed
to test IOMMU-capable mdev. We don't have real hardware on market, there
is no way to test the VFIO extensions for IOMMU-capable mdev. Wrapping a
PCI device in mdev can test the VFIO extensions well as it has hardware
enforce DMA isolation. Thus makes it possible to test the extensions in VFIO.

>
> >
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Lu Baolu <baolu.lu@linux.intel.com>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > ---
> > samples/Kconfig | 10 +
> > samples/Makefile | 1 +
> > samples/vfio-mdev-pci/Makefile | 4 +
> > samples/vfio-mdev-pci/vfio_mdev_pci.c | 397
> ++++++++++++++++++++++++++++++++++
> > 4 files changed, 412 insertions(+)
> > create mode 100644 samples/vfio-mdev-pci/Makefile
> > create mode 100644 samples/vfio-mdev-pci/vfio_mdev_pci.c
> >
> > diff --git a/samples/Kconfig b/samples/Kconfig
> > index 9d236c3..50d207c 100644
> > --- a/samples/Kconfig
> > +++ b/samples/Kconfig
> > @@ -190,5 +190,15 @@ config SAMPLE_INTEL_MEI
> > help
> > Build a sample program to work with mei device.
> >
> > +config SAMPLE_VFIO_MDEV_PCI
> > + tristate "Sample driver for wrapping PCI device as a mdev"
> > + select VFIO_PCI_COMMON
> > + select VFIO_PCI
>
> Why does this still need to select VFIO_PCI? Shouldn't all needed
> infrastructure rather be covered by VFIO_PCI_COMMON already?

VFIO_PCI_COMMON is supposed to be the dependency of both VFIO_PCI and
SAMPLE_VFIO_MDEV_PCI. However, the source code of VFIO_PCI_COMMON are
under drivers/vfio/pci which is compiled per the configuration of VFIO_PCI.
Besides of letting SAMPLE_VFIO_MDEV_PCI select VFIO_PCI, I can also add
a line in drivers/vfio/Makefile to make the source code under drivers/vfio/pci
to be compiled when either VFIO_PCI or VFIO_PCI_COMMON are configed. But
I'm afraid it is a bit ugly. So I choose to let SAMPLE_VFIO_MDEV_PCI select
VFIO_PCI. If you have other idea, I would be pleased to
know it. :-)

>
> > + depends on VFIO_MDEV && VFIO_MDEV_DEVICE
>
> VFIO_MDEV_DEVICE already depends on VFIO_MDEV. But maybe also make this
> depend on PCI?
>
> > + help
> > + Sample driver for wrapping a PCI device as a mdev. Once bound to
> > + this driver, device passthru should through mdev path.
>
> "A PCI device bound to this driver will be assigned through the
> mediated device framework."
>
> ?

Maybe I should have mentioned it as "A PCI device bound to this
sample driver should follow the passthru steps for mdevs as showed
in Documentation/driver-api/vfio-mediated-device.rst."

Does it make more sense?

Thanks,
Yi Liu

>
> > +
> > + If you don't know what to do here, say N.
> >
> > endif # SAMPLES