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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 10/03/2015 12:20 AM, Alex Williamson wrote:
> On Fri, 2015-10-02 at 17:58 +1000, Alexey Kardashevskiy wrote:
>> On 09/10/2015 04:34 AM, Alex Williamson wrote:
>>> On Wed, 2015-09-09 at 17:17 +1000, Alexey Kardashevskiy wrote:
>>>> 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?
>>>
>>> See the patch series I just posted that eliminates use of target page
>>> size in quirks and then tell me if/why you still need to avoid quirks on
>>> POWER.  Thanks,
>>
>> I pulled the latest upstream which has "VFIO updates 2015-09-23", I presume
>> this is the patchset you mentioned (which I am not sure about).
>>
>> The vfio_probe_nvidia_bar0_quirk() fails by exact same reason - the offset
>> (0x88000) is not host page size (0x10000) aligned. It used to be
>> &TARGET_PAGE_MASK, now there is no alignment. &qemu_real_host_page_mask
>> would work if I wanted the quirk but as you mentioned before, it is a more
>> straight approach if we just disable quirks when we do not need them.
>
> Sorry, I don't accept this answer.  We have another quirk initialized
> from vfio_probe_nvidia_bar0_quirk() that places a quirk at 0x1800.  This
> is not aligned with the host page size on x86 (0x1000), but it works as
> expected.

Ah. My ignorance. I did not realize that. So the problem is actually in 
kvm_set_phys_mem() in kvm-all.c which aligns to TARGET_PAGE_SIZE (not 
qemu_real_host_page_size), this is why it works for 4K pages. I'll try 
patching kvm-all.c.


> This suggests that there's something wrong with how your
> platform handles memory regions and if we need to worry about host page
> sizes every time we want to drop a quirk into an MMIO region, this
> problem is going to continue to plague us.  The reason for disabling
> this quirk should be performance or scalability, not functionality.  The
> functionality problem needs to be found and fixed.  Thanks,

I agree. I was only going that direction because you suggested disabling 
quirks on the platform which does not need it.


-- 
Alexey