Re: [Qemu-devel] [RFC PATCH] qemu pci: pci_add_capability enhancement to prevent damaging config space

[Jason Baron <jbaron@redhat.com>]

On Fri, May 11, 2012 at 04:45:21PM +1000, Alexey Kardashevskiy wrote:
> Normally the pci_add_capability is called on devices to add new
> capability. This is ok for emulated devices which capabilities list
> is being built by QEMU.
> 
> In the case of VFIO the capability may already exist and adding new
> capability into the beginning of the linked list may create a loop.

Hi,

I don't quite understand how we get a loop, if 'offset' is supplied to
'pci_add_capability' and there is an overlap we get -EINVAL. Otherwise,
we are adding the capability in a new empty space. So, I see how we
could get the capability in the list twice, but not how there is a loop.
what am I missing?

Thanks,

-Jason

> 
> For example, the old code destroys the following config
> of PCIe Intel E1000E:
> 
> before adding PCI_CAP_ID_MSI (0x05):
> 0x34: 0xC8
> 0xC8: 0x01 0xD0
> 0xD0: 0x05 0xE0
> 0xE0: 0x10 0x00
> 
> after:
> 0x34: 0xD0
> 0xC8: 0x01 0xD0
> 0xD0: 0x05 0xC8
> 0xE0: 0x10 0x00
> 
> As result capabilities 0x01 and 0x05 point to each other.
> 
> The proposed patch does not change capability pointers when
> the same type capability is about to add.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/pci.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index aa0c0b8..1f7c924 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>      }
> 
>      config = pdev->config + offset;
> -    config[PCI_CAP_LIST_ID] = cap_id;
> -    config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
> -    pdev->config[PCI_CAPABILITY_LIST] = offset;
> -    pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
> +    if (config[PCI_CAP_LIST_ID] != cap_id) {
> +        config[PCI_CAP_LIST_ID] = cap_id;
> +        config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
> +        pdev->config[PCI_CAPABILITY_LIST] = offset;
> +        pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
> +    }
>      memset(pdev->used + offset, 0xFF, size);
>      /* Make capability read-only by default */
>      memset(pdev->wmask + offset, 0, size);
> 
> 
> -- 
> Alexey
>