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

[Blue Swirl]

On Wed, Sep 26, 2012 at 6:51 PM, Matt Renzelmann <address@hidden> wrote:
> The current implementation of pci_find_space does not correctly align
> PCI capabilities in the PCI configuration space.  It also does not
> support PCI-Express devices.  This patch fixes these issues.
>
> Thanks to Alex Williamson for feedback.
>
> Signed-off-by: Matt Renzelmann <address@hidden>
> ---
>
> This version adds the assertions to pci_find_space and removes the
> assertion from pci_find_express_space.
>
>  hw/pci.c |   35 +++++++++++++++++++++++++++--------
>  1 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index f855cf3..7050596 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1626,19 +1626,38 @@ 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, uint32_t start,
> +                          uint32_t end, uint32_t size)
>  {
> -    int config_size = pci_config_size(pdev);
> -    int offset = PCI_CONFIG_HEADER_SIZE;
> +    int offset = start;
>      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[start];
> +
> +    assert(pci_config_size(pdev) >= end);
> +    assert(!(start & 0x3));
> +
> +    /* This approach ensures the capability is dword-aligned, as
> +       required by the PCI and PCI-E specifications */
> +    for (i = start; i < end; i += 4, dword_used++) {
> +        if (*dword_used)

Please add braces.

> +            offset = i + 4;
> +        else if (i - offset + 4 >= size)

Also here to the 'if' part.

>              return offset;
> +    }
> +
>      return 0;
>  }
>
> +static int pci_find_legacy_space(PCIDevice *pdev, uint8_t size) {
> +    return pci_find_space(pdev, PCI_CONFIG_HEADER_SIZE,
> +                          PCI_CONFIG_SPACE_SIZE, size);
> +}
> +
> +static int pci_find_express_space(PCIDevice *pdev, uint16_t size) {
> +    return pci_find_space(pdev, PCI_CONFIG_SPACE_SIZE,
> +                          PCIE_CONFIG_SPACE_SIZE, size);
> +}
> +
>  static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
>                                          uint8_t *prev_p)
>  {
> @@ -1826,7 +1845,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_legacy_space(pdev, size);
>          if (!offset) {
>              return -ENOSPC;
>          }
> --
> 1.7.5.4
>
>