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

[Alexey Kardashevskiy]

On 07/20/2015 03:15 AM, Alex Williamson wrote:
> 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.

But the memory API cannot handle it right now, right?

>  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.

Quirks do not install - KVM fails to register these memory regions. And 
when I fix this, they do not for that another unknown reason, so it is not 
probably, it is definitely :)


>  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,



Regarding automatic disabling of quirks on POWER -  I'd love to do that but 
how can I do this without adding a property? #ifdef PPC64 in hw/vfio/pci.c? 
We were avoiding this.




> 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,


--
Alexey