Re: [Qemu-devel] [PATCH v3] Align PCI capabilities in pci_find_space

[Alex Williamson]

On Wed, 2012-09-26 at 10:30 -0500, address@hidden wrote:
> From: Matt Renzelmann <address@hidden>
> 
> The current implementation of pci_find_space does not correctly align
> PCI capabilities in the PCI configuration space.  It also does not
> distinguish PCI and PCI-Express devices.  This patch fixes these
> issues.
> 
> Thanks to Alex Williamson for continuing feedback.
> 
> Signed-off-by: Matt Renzelmann <address@hidden>
> ---
> 
> In this patch, I've revised the pci_find_space function as suggested
> (more-or-less).  I searched for calls to pci_add_capability, and at
> this time, most rely only on capabilities that fit in the PCI config
> space.  More importantly, almost all specify the capability offset
> instead of relying on pci_find_space, so this change does not impact
> any calls that specify an offset manually.  However, it's important to
> double-check that there are no calls from PCI-E virtual devices to
> pci_add_capability that both:
> 
> (a) relied on pci_find_space to find them space
> 
> (b) needed the PCI-E extended config space searched in addition to the
> PCI space
> 
> as these would break with this patch. Here is the list of files that
> refer to pcie_cap_init:
> 
> ./hw/pcie.c
> ./hw/pcie.h
> ./hw/ioh3420.c
> ./hw/usb/hcd-xhci.c
> ./hw/xio3130_upstream.c
> ./hw/xio3130_downstream.c
> 
> The goal of this search was simply to find PCI-E devices--there may be
> a better way.  The next list contain calls to pci_add_capability:
> 
> ./hw/pci_bridge.c
> ./hw/shpc.c
> ./hw/pcie.c
> ./hw/kvm/pci-assign.c
> ./hw/msi.c
> ./hw/pci.c
> ./hw/ide/ich.c
> ./hw/pci.h
> ./hw/eepro100.c
> ./hw/msix.c
> ./hw/slotid_cap.c
> 
> 
>  hw/pci.c |   28 +++++++++++++++++++++-------
>  1 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index f855cf3..2217dda 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1626,16 +1626,30 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, 
> const char *name)
>      return pci_create_simple_multifunction(bus, devfn, false, name);
>  }
>  
> -static int pci_find_space(PCIDevice *pdev, uint8_t size)
> +static int pci_find_space(PCIDevice *pdev, uint8_t size, bool include_pcie)
>  {
> -    int config_size = pci_config_size(pdev);
> +    int config_size;
>      int offset = PCI_CONFIG_HEADER_SIZE;
>      int i;
> -    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
> -        if (pdev->used[i])
> -            offset = i + 1;
> -        else if (i - offset + 1 == size)
> +    uint32_t *dword_used = &pdev->used[PCI_CONFIG_HEADER_SIZE];
> +
> +    if (include_pcie) {
> +        assert (pci_config_size(pdev) >= PCIE_CONFIG_SPACE_SIZE);
> +        config_size = PCIE_CONFIG_SPACE_SIZE;
> +    } else {
> +        config_size = PCI_CONFIG_SPACE_SIZE;
> +    }
> +
> +    /* This approach ensures the capability is dword-aligned, as
> +       required by the PCI specification */
> +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i += 4, dword_used++) {

I don't believe there's ever a case where a driver would want space and
not care if it's in standard or extended config space.  They'll want one
or the other.  So we'd be searching two distinct ranges.  Thanks,

Alex

> +        if (*dword_used) {
> +            offset = i + 4;
> +        } else if (i - offset + 4 >= size) {
>              return offset;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -1826,7 +1840,7 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>      int i, overlapping_cap;
>  
>      if (!offset) {
> -        offset = pci_find_space(pdev, size);
> +        offset = pci_find_space(pdev, size, false);
>          if (!offset) {
>              return -ENOSPC;
>          }