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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <aik@ozlabs.ru>
>>> ---
>>>
>>> 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