| Message ID | 1437293970-6727-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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. > --- > 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, Alex
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. 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. 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, 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, > > Alex > >
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,
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, > >
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, Alex
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.
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
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.
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),
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. --- hw/vfio/pci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)