Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2016-01-20 at 18:11 +0000, Daniel P. Berrange wrote:
> On Wed, Jan 20, 2016 at 11:06:55AM -0700, Alex Williamson wrote:
> > vfio-pci currently requires a host= parameter, which comes in the
> > form of a PCI address in [domain:]<bus:slot.function> notation.  We
> > expect to find a matching entry in sysfs for that under
> > /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> > defines the host= parameter to be a string, which can be matched
> > directly under /sys/bus/platform/devices/.  On the PCI side, we
> > have
> > some interest in using vfio to expose vGPU devices.  These are not
> > actual discrete PCI devices, so they don't have a compatible host
> > PCI
> > bus address or a device link where QEMU wants to look for
> > it.  There's
> > also really no requirement that vfio can only be used to expose
> > physical devices, a new vfio bus and iommu driver could expose a
> > completely emulated device.  To fit within the vfio framework, it
> > would need a kernel struct device and associated IOMMU group, but
> > those are easy constraints to manage.
> > 
> > To support such devices, which would include vGPUs, that honor the
> > VFIO PCI programming API, but are not necessarily backed by a
> > unique
> > PCI address, add support for specifying any device in sysfs.  The
> > vfio API already has support for probing the device type to ensure
> > compatibility with either vfio-pci or vfio-platform.
> > 
> > With this, a vfio-pci device could either be specified as:
> > 
> > -device vfio-pci,host=02:00.0
> > 
> > or
> > 
> > -device vfio-
> > pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> > 
> > or even
> > 
> > -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> > 
> > When vGPU support comes along, this might look something more like:
> > 
> > -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@000
> > 0:00:02.0
> > 
> > NB - This is only a made up example path, but it should be noted
> > that
> > the device namespace is global for vfio, a virtual device cannot
> > overlap with existing namespaces and should not create a name prone
> > to
> > conflict, such as a simple instance number.
> > 
> > The same changes is made for vfio-platform but is only compile
> > tested.
> > In both cases, specifying sysfsdev has precedence over the old host
> > option.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  hw/vfio/pci.c                 |  130 +++++++++++++++++----------
> > --------------
> >  hw/vfio/platform.c            |   55 ++++++++++-------
> >  include/hw/vfio/vfio-common.h |    1 
> >  3 files changed, 86 insertions(+), 100 deletions(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 1fb868c..bfe4215 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -880,12 +880,8 @@ static void vfio_pci_size_rom(VFIOPCIDevice
> > *vdev)
> >      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> >          /* Since pci handles romfile, just print a message and
> > return */
> >          if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> > -            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> > -                         "is known to cause system instability
> > issues during "
> > -                         "option rom execution. "
> > -                         "Proceeding anyway since user specified
> > romfile\n",
> > -                         vdev->host.domain, vdev->host.bus, vdev-
> > >host.slot,
> > -                         vdev->host.function);
> > +            error_printf("Warning : Device at %s is known to cause
> > system instability issues during option rom execution. Proceeding
> > anyway since user specified romfile\n",
> > +                         vdev->vbasedev.name);
> 
> This error message should really be split across lines like
> the original code was. Likewise for other cases in this patch

kernel and QEMU development disagree here and I tend to favor the
kernel's argument that if a user is inclined enough to go grep through
the source to find an error message, it should work.  Breaking printed
error messages at arbitrary lengths to fit an 80 column window makes
that more difficult.  Is there sufficient passion for a hard 80 column
limit on printed strings in QEMU to ignore that useful property?
Thanks,

Alex