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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2012-09-25 at 20:01 -0500, mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> The current implementation of pci_find_space does not correctly align
> PCI capabilities in the PCI configuration space.  This patch fixes
> this issue.
> 
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
> 
> Alex Williamson <alex.williamson@redhat.com> wrote:
> > I think you could just search every 4th byte.  In fact, this whole used
> > byte-map could be turned into a single uint64_t bitmap for standard
> > config space.  Thanks,
> 
> I've not tested this version of the patch, in contrast to the last, so
> I'm a bit less confident of its correctness.  I did not reimplement it
> as suggested as I'm not that familiar with this code, and instead just
> applied the every 4th byte strategy.
> 
>  hw/pci.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index f855cf3..e99866a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1631,11 +1631,15 @@ static int pci_find_space(PCIDevice *pdev, uint8_t size)
>      int config_size = pci_config_size(pdev);
>      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)
> +
> +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i += 4) {
> +        if (pdev->used[i]) {
> +            offset = i + 4;
> +        } else if (i - offset + 1 == size) {

This test needs to change as well.  Looks like it should now be:

 (i - offset + 4 >= size)

Whereas we were previously calculating the difference from the offset to
the current pointer plus the current unused byte, we're now assuming the
current dword is empty because we're only handing out dword aligned
offsets and it would be broken for something to not mark the first entry
used.  Probably worthwhile to also add a comment noting the PCI spec
requires dword alignment for capabilities.  Thanks,

Alex

>              return offset;
> +        }
> +    }
> +
>      return 0;
>  }
>