Re: [RFC PATCH 0/7] vfio/pci: SR-IOV support

[Jerin Jacob]

On Wed, Feb 5, 2020 at 4:35 AM Alex Williamson
<alex.williamson@xxxxxxxxxx> wrote:
>
> There seems to be an ongoing desire to use userspace, vfio-based
> drivers for both SR-IOV PF and VF devices.  The fundamental issue
> with this concept is that the VF is not fully independent of the PF
> driver.  Minimally the PF driver might be able to deny service to the
> VF, VF data paths might be dependent on the state of the PF device,
> or the PF my have some degree of ability to inspect or manipulate the
> VF data.  It therefore would seem irresponsible to unleash VFs onto
> the system, managed by a user owned PF.
>
> We address this in a few ways in this series.  First, we can use a bus
> notifier and the driver_override facility to make sure VFs are bound
> to the vfio-pci driver by default.  This should eliminate the chance
> that a VF is accidentally bound and used by host drivers.  We don't
> however remove the ability for a host admin to change this override.
>
> The next issue we need to address is how we let userspace drivers
> opt-in to this participation with the PF driver.  We do not want an
> admin to be able to unwittingly assign one of these VFs to a tenant
> that isn't working in collaboration with the PF driver.  We could use
> IOMMU grouping, but this seems to push too far towards tightly coupled
> PF and VF drivers.  This series introduces a "VF token", implemented
> as a UUID, as a shared secret between PF and VF drivers.  The token
> needs to be set by the PF driver and used as part of the device
> matching by the VF driver.  Provisions in the code also account for
> restarting the PF driver with active VF drivers, requiring the PF to
> use the current token to re-gain access to the PF.

Thanks Alex for the series. DPDK realizes this use-case through, an out of
tree igb_uio module, for non VFIO devices. Supporting this use case, with
VFIO, will be a great enhancement for DPDK as we are planning to
get rid of out of tree modules any focus only on userspace aspects.

>From the DPDK perspective, we have following use-cases

1) VF representer or OVS/vSwitch  use cases where
DPDK PF acts as an HW switch to steer traffic to VF
using the rte_flow library backed by HW CAMs.

2) Unlike, other PCI class of devices, Network class of PCIe devices
would have additional
capability on the PF devices such as promiscuous mode support etc
leverage that in DPDK
PF and VF use cases.

That would boil down to the use of the following topology.
a)  PF bound to DPDK/VFIO  and  VF bound to Linux
b)  PF bound to DPDK/VFIO  and  VF bound to DPDK/VFIO

Tested the use case (a) and it works this patch. Tested use case(b), it
works with patch provided both PF and VF under the same application.

Regarding the use case where  PF bound to DPDK/VFIO and
VF bound to DPDK/VFIO are _two different_ processes then sharing the UUID
will be a little tricky thing in terms of usage. But if that is the
purpose of bringing
UUID to the equation then it fine.

Overall this series looks good to me.  We can test the next non-RFC
series and give
Tested-by by after testing with DPDK.


>
> The above solutions introduce a bit of a modification to the VFIO ABI
> and an additional ABI extension.  The modification is that the
> VFIO_GROUP_GET_DEVICE_FD ioctl is specified to require a char string
> from the user providing the device name.  For this solution, we extend
> the syntax to allow the device name followed by key/value pairs.  In
> this case we add "vf_token=3e7e882e-1daf-417f-ad8d-882eea5ee337", for
> example.  These options are expected to be space separated.  Matching
> these key/value pairs is entirely left to the vfio bus driver (ex.
> vfio-pci) and the internal ops structure is extended to allow this
> optional support.  This extension should be fully backwards compatible
> to existing userspace, such code will simply fail to open these newly
> exposed devices, as intended.
>
> I've been debating whether instead of the above we should allow the
> user to get the device fd as normal, but restrict the interfaces until
> the user authenticates, but I'm afraid this would be a less backwards
> compatible solution.  It would be just as unclear to the user why a
> device read/write/mmap/ioctl failed as it might be to why getting the
> device fd could fail.  However in the latter case, I believe we do a
> better job of restricting how far userspace code might go before they
> ultimately fail.  I'd welcome discussion in the space, and or course
> the extension of the GET_DEVICE_FD string.
>
> Finally, the user needs to be able to set a VF token.  I add a
> VFIO_DEVICE_FEATURE ioctl for this that's meant to be reusable for
> getting, setting, and probing arbitrary features of a device.
>
> I'll reply to this cover letter with a very basic example of a QEMU
> update to support this interface, though I haven't found a device yet
> that behaves well with the PF running in one VM with the VF in
> another, or really even just a PF running in a VM with SR-IOV enabled.
> I know these devices exist though, and I suspect QEMU will not be the
> primary user of this support for now, but this behavior reaffirms my
> concerns to prevent mis-use.
>
> Please comment.  In particular, does this approach meet the DPDK needs
> for userspace PF and VF drivers, with the hopefully minor hurdle of
> sharing a token between drivers.  The token is of course left to
> userspace how to manage, and might be static (and not very secret) for
> a given set of drivers.  Thanks,
>
> Alex
>
> ---
>
> Alex Williamson (7):
>       vfio: Include optional device match in vfio_device_ops callbacks
>       vfio/pci: Implement match ops
>       vfio/pci: Introduce VF token
>       vfio: Introduce VFIO_DEVICE_FEATURE ioctl and first user
>       vfio/pci: Add sriov_configure support
>       vfio/pci: Remove dev_fmt definition
>       vfio/pci: Cleanup .probe() exit paths
>
>
>  drivers/vfio/pci/vfio_pci.c         |  315 ++++++++++++++++++++++++++++++++---
>  drivers/vfio/pci/vfio_pci_private.h |   10 +
>  drivers/vfio/vfio.c                 |   19 ++
>  include/linux/vfio.h                |    3
>  include/uapi/linux/vfio.h           |   37 ++++
>  5 files changed, 356 insertions(+), 28 deletions(-)
>