Re: [Qemu-devel] [PATCH qemu] vfio_pci: Allow disabling quirks

[Alex Williamson]

On Sun, 2015-07-19 at 06:50 -0600, Alex Williamson wrote:
> On Sun, 2015-07-19 at 18:19 +1000, Alexey Kardashevskiy wrote:
> > The existing quirks aim config space and MSIX BAR accesses interception.
> > These are not always needed, for example, on pseries machines,
> > config space and MSI/MSIX configuration are handled by hypervisor.
> > 
> > This adds a "quirks" property to control whether to enable quirks or not;
> > the property is set to "true" by default.
> > 
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > ---
> > 
> > Helps to get
> > VGA compatible controller: NVIDIA Corporation GM107GL [Quadro K2200] (rev 
> > a2)
> > (which does not need the quirk anyway) working on POWER8 system.

BTW, as I was working on the rtl quirk last week, I re-realized
something; page size doesn't matter for quirks.  If we want to carve a
4k hole for a PCI extended config space window, we can do that
regardless of the host page size.  The rtl quirk only carves out an 8
byte window.  It's the Memory API's problem to figure out the extent of
the region that needs to fault into QEMU and which parts go through the
quirk vs the slow backing of the BAR.  That's why we create the slow
backing across the entire BAR.  So if this quirk isn't working for you,
page size is probably not the reason.  That said, there are some
gratuitous uses of target page size in the quirk code.  Part of it is
just a convenience factor for packing data structures, part of it is
completely unnecessary, like the TARGET_PAGE_ALIGN setting up the quirk
in question here.  Thanks,

Alex

> > ---
> >  hw/vfio/pci.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 2ed877f..ba47301 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice {
> >      bool has_flr;
> >      bool has_pm_reset;
> >      bool rom_read_failed;
> > +    bool allow_quirks;
> >  } VFIOPCIDevice;
> >  
> >  typedef struct VFIORomBlacklistEntry {
> > @@ -2442,7 +2443,9 @@ static void vfio_map_bar(VFIOPCIDevice *vdev, int nr)
> >          }
> >      }
> >  
> > -    vfio_bar_quirk_setup(vdev, nr);
> > +    if (vdev->allow_quirks) {
> > +        vfio_bar_quirk_setup(vdev, nr);
> > +    }
> >  }
> >  
> >  static void vfio_map_bars(VFIOPCIDevice *vdev)
> > @@ -3753,6 +3756,7 @@ static Property vfio_pci_dev_properties[] = {
> >                      VFIO_FEATURE_ENABLE_REQ_BIT, true),
> >      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> >      DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
> > +    DEFINE_PROP_BOOL("quirks", VFIOPCIDevice, allow_quirks, true),
> >      /*
> >       * TODO - support passed fds... is this necessary?
> >       * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
> 
> 
> NAK, if you don't need it for power then make it automatically disabled
> for power.  Otherwise you're just pushing the burden onto the
> user/libvirt to know when to use this option.  Thanks,
> 
> Alex
> 
>