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

[Alexander Graf]

On 11.05.2012, at 08:45, 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.
> 
> 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 <address@hidden>
> ---
> 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) {

This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop 
through all capabilities, check if the one you want to add is there already and 
if so either

  * replace the existing one or
  * drop out and not write the new one in.

I'm not sure which way would be more natural.

> +        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