From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz6QJ-0001fq-OX for qemu-devel@nongnu.org; Wed, 18 Nov 2015 12:21:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zz6QG-0001Vp-Jt for qemu-devel@nongnu.org; Wed, 18 Nov 2015 12:21:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47904) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz6QG-0001Vf-C7 for qemu-devel@nongnu.org; Wed, 18 Nov 2015 12:21:32 -0500 References: <20151117234028.23385.99569.stgit@gimli.home> <564C46A6.502@redhat.com> <1447862598.4697.14.camel@redhat.com> From: Laszlo Ersek Message-ID: <564CB398.9010009@redhat.com> Date: Wed, 18 Nov 2015 18:21:28 +0100 MIME-Version: 1.0 In-Reply-To: <1447862598.4697.14.camel@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Only quirk to size of PCI config space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: Ronnie Swanink , qemu-devel@nongnu.org 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 >>> Reported-by: Ronnie Swanink >>> --- >>> 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 >> >> >> 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 > > 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 > 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 > Reviewed-by: Laszlo Ersek > Signed-off-by: Alex Williamson > > Looks great, thanks! Laszlo