On 11/18/15 17:03, Alex Williamson wrote: > On Wed, 2015-11-18 at 10:36 +0100, Laszlo Ersek wrote: >> On 11/18/15 00:41, Alex Williamson wrote: >>> For quirks that support the full PCIe extended config space, limit the >>> quirk to only the size of config space available through vfio. This >>> allows host systems with broken MMCONFIG regions to still make use of >>> these quirks without generating bad address faults trying to access >>> beyond the end of config space exposed through vfio. This may expose >>> direct access to parts of extended config space that we'd prefer not >>> to expose, but that's why we have an IOMMU. >>> >>> Signed-off-by: Alex Williamson <alex.william...@redhat.com> >>> Reported-by: Ronnie Swanink <ron...@ronnieswanink.nl> >>> --- >>> hw/vfio/pci-quirks.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c >>> index 30c68a1..e117c41 100644 >>> --- a/hw/vfio/pci-quirks.c >>> +++ b/hw/vfio/pci-quirks.c >>> @@ -328,7 +328,7 @@ static void vfio_probe_ati_bar4_quirk(VFIOPCIDevice >>> *vdev, int nr) >>> window->data_offset = 4; >>> window->nr_matches = 1; >>> window->matches[0].match = 0x4000; >>> - window->matches[0].mask = PCIE_CONFIG_SPACE_SIZE - 1; >>> + window->matches[0].mask = vdev->config_size - 1; >>> window->bar = nr; >>> window->addr_mem = &quirk->mem[0]; >>> window->data_mem = &quirk->mem[1]; >>> @@ -674,7 +674,7 @@ static void vfio_probe_nvidia_bar5_quirk(VFIOPCIDevice >>> *vdev, int nr) >>> window->matches[0].match = 0x1800; >>> window->matches[0].mask = PCI_CONFIG_SPACE_SIZE - 1; >>> window->matches[1].match = 0x88000; >>> - window->matches[1].mask = PCIE_CONFIG_SPACE_SIZE - 1; >>> + window->matches[1].mask = vdev->config_size - 1; >>> window->bar = nr; >>> window->addr_mem = bar5->addr_mem = &quirk->mem[0]; >>> window->data_mem = bar5->data_mem = &quirk->mem[1]; >>> @@ -765,7 +765,7 @@ static void vfio_probe_nvidia_bar0_quirk(VFIOPCIDevice >>> *vdev, int nr) >>> memory_region_init_io(mirror->mem, OBJECT(vdev), >>> &vfio_nvidia_mirror_quirk, mirror, >>> "vfio-nvidia-bar0-88000-mirror-quirk", >>> - PCIE_CONFIG_SPACE_SIZE); >>> + vdev->config_size); >>> memory_region_add_subregion_overlap(&vdev->bars[nr].region.mem, >>> mirror->offset, mirror->mem, 1); >>> >>> >>> >> >> $ git log -- hw/vfio/pci-quirks.c | grep Reviewed-by >> <nothing> >> >> Okay, I guess my review won't be deemed immediately unnecessary then. :) > > Reviews are very much appreciated. > >> (1) Please add to the commit message: >> >> Link: https://www.redhat.com/archives/vfio-users/2015-November/msg00192.html >> >> With that reference: >> >> Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > Done > >> (2) Also, one question just to make sure I understand right: the last >> sentence of the commit message means that the left-inclusive, >> right-exclusive config space offset range >> >> [vdev->config_size, PCIE_CONFIG_SPACE_SIZE) >> >> is now directly available to the guest, without QEMU noticing; but, >> we're not worried about that, because the guest can't abuse that freedom >> anyway for doing arbitrary DMA. Is that right? >> >> If so, then I propose another update to the commit message (replacing >> the last sentence): >> >> This may grant the guest direct access to trailing parts of >> extended config space that we'd prefer not to expose, but that's >> why we have an IOMMU (the guest can't abuse said config space access >> for arbitrary DMA). >> >> Perhaps the above is trivial for you (assuming it is correct at all...); >> it may not be trivial for others. > > How's this?: > > commit b27c4f5da92a1732a77ddbe98571583a4eac1e14 > Author: Alex Williamson <alex.william...@redhat.com> > Date: Tue Nov 17 16:37:38 2015 -0700 > > vfio/pci-quirks: Only quirk to size of PCI config space > > For quirks that support the full PCIe extended config space, limit the > quirk to only the size of config space available through vfio. This > allows host systems with broken MMCONFIG regions to still make use of > these quirks without generating bad address faults trying to access > beyond the end of config space exposed through vfio. This may expose > direct access to the mirror of extended config space, only trapping > the sub-range of standard config space, but allowing this makes the > quirk, and thus the device, functional. We expect that only device > specific accesses make use of the mirror, not general extended PCI > capability accesses, so any virtualization in this space is likely > unnecessary anyway, and the device is still IOMMU isolated, so it > should only be able to hurt itself through any bogus configurations > enabled by this space. > > Link: > https://www.redhat.com/archives/vfio-users/2015-November/msg00192.html > Reported-by: Ronnie Swanink <ron...@ronnieswanink.nl> > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > Signed-off-by: Alex Williamson <alex.william...@redhat.com> > >
Looks great, thanks! Laszlo