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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 07/20/2015 12:40 PM, Alexey Kardashevskiy wrote:
> 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.

Ping?

The only proper automatic disabling on POWER I can think of would be:
1) (still) add a "allow_quirks" property
2) in the pseries machine init code, set the property in the compat_props 
to "false".

But this still requires a property. Better ideas?


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