[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Only quirk to size of PCI conf
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Only quirk to size of PCI config space |
Date: |
Wed, 18 Nov 2015 18:21:28 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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 <address@hidden>
>>> Reported-by: Ronnie Swanink <address@hidden>
>>> ---
>>> 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 <address@hidden>
>
> 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 <address@hidden>
> 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 <address@hidden>
> Reviewed-by: Laszlo Ersek <address@hidden>
> Signed-off-by: Alex Williamson <address@hidden>
>
>
Looks great, thanks!
Laszlo