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

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

[mjr]

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) {
             return offset;
+        }
+    }
+
     return 0;
 }
 
-- 
1.7.5.4

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

[Alex Williamson]

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

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

[Alex Williamson]

On Tue, 2012-09-25 at 21:08 -0600, Alex Williamson wrote:
> 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,

BTW, rather than assume the rest of the dword is empty, we could just
check each dword instead of each byte, something like

uint32_t *dword_used = &pdev->used[PCI_CONFIG_HEADER_SIZE];

for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i +=4, dword_used++) {
    if (*dword_used) {
        offset = i + 4;
    } else if (i - offset + 4 >= size) {
        return offset;
    }
}

It also occurs to me that this function is broken for PCIe devices as we
should stop at PCI_CONFIG_SPACE_SIZE instead of config_size.  There
should be a separate allocator for extended config space, or a flag to
this function to indicate standard or extended.  Thanks,

Alex

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