Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Only quirk to size of PCI config space

[Laszlo Ersek <lersek@redhat.com>]

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.williamson@redhat.com>
>>> Reported-by: Ronnie Swanink <ronnie@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 <lersek@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.williamson@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 <ronnie@ronnieswanink.nl>
>     Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> 

Looks great, thanks!
Laszlo