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

[Alex Williamson <alex.williamson@redhat.com>]

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

Alex