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

[Michael S. Tsirkin]

On Sat, Oct 20, 2012 at 04:01:12PM -0500, Matt Renzelmann 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>
> ---
> 
> Re-sending to add CC Michael S. Tsirkin <address@hidden>.  Thanks
> Andreas for pointing out my mistake.
> 
>  hw/pci.c |   36 ++++++++++++++++++++++++++++--------
>  1 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2ca6ff6..4b617f6 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1644,19 +1644,39 @@ 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) {
> +            offset = i + 4;
> +        } else if (i - offset + 4 >= size) {
>              return offset;
> +        }
> +    }
> +
>      return 0;
>  }

I agree ability to get misaligned capabilities is a bug.  Thanks for
reorting this.  But it seems easier to fix just by aligning size.  See
patch below.


>  
> +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);
> +}

I think it makes more sense to make pci_find_space imply
legacy and add a new API for express. This is exactly what patches
that Jason Baron posted do, so I'll apply them instead.

> +
> +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);
> +}
> +

This is dead code I think, it's probably not a good idea to
add yet at this stage.

>  static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
>                                          uint8_t *prev_p)
>  {
> @@ -1844,7 +1864,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;
>          }

Below is what I applied. Thanks for the report!

--->

pci: make each capability DWORD aligned

PCI spec (see e.g. 6.7 Capabilities List in spec rev 3.0)
requires that each capability is DWORD aligned.
Ensure this when allocating space by rounding size up to 4.

Signed-off-by: Michael S. Tsirkin <address@hidden>
Reported-by: Matt Renzelmann <address@hidden>

diff --git a/hw/pci.c b/hw/pci.c
index 6a66b32..28fdb19 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1883,7 +1883,7 @@ int pci_add_capability(PCIDevice *pdev, uint8_t 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);
+    memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));
     /* Make capability read-only by default */
     memset(pdev->wmask + offset, 0, size);
     /* Check capability by default */
@@ -1903,7 +1903,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, 
uint8_t size)
     memset(pdev->w1cmask + offset, 0, size);
     /* Clear cmask as device-specific registers can't be checked */
     memset(pdev->cmask + offset, 0, size);
-    memset(pdev->used + offset, 0, size);
+    memset(pdev->used + offset, 0, QEMU_ALIGN_UP(size, 4));
 
     if (!pdev->config[PCI_CAPABILITY_LIST])
         pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;