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

[Alex Williamson]

On Wed, 2012-09-26 at 11:50 -0500, Matt Renzelmann wrote:
> > >
> > >  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
> 
> Ah, that makes sense, so would something like this work?  I can re-send as a 
> patch once we've got it.
> 
> static int pci_find_space(PCIDevice *pdev, uint8_t size, bool pcie_space)
> {
>     int config_base;
>     int config_size;
>     int offset = PCI_CONFIG_HEADER_SIZE;
>     int i;
>     uint32_t *dword_used = &pdev->used[PCI_CONFIG_HEADER_SIZE];

This needs to change too.

Is there a different alignment requirement for pcie?  Seems like you
might be better off creating some helper like:

pci_find_space(PCIDevice *pdev, unsigned start, unsigned size, unsigned
align)

Then a pci_find_legacy_space and pci_find_express_space could both call
into it.  Thanks,

Alex

> 
>     if (pcie_space) {
>         assert (pci_config_size(pdev) >= PCIE_CONFIG_SPACE_SIZE);
>         config_base = PCI_CONFIG_SPACE_SIZE;
>         config_size = PCIE_CONFIG_SPACE_SIZE;
>     } else {        
>         config_base = PCI_CONFIG_HEADER_SIZE;
>         config_size = PCI_CONFIG_SPACE_SIZE;
>     }
> 
>     /* This approach ensures the capability is dword-aligned, as              
>                                           
>        required by the PCI specification */
>     for (i = config_base; i < config_size; i += 4, dword_used++) {            
>            
>         if (*dword_used)
>             offset = i + 4;
>         else if (i - offset + 4 >= size)
>             return offset;
>     }        
> 
>     return 0;
> }
> 
> Thanks for all your help with this,
> Matt
> 
> > 
> > > +        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;
> > >          }
> > 
> > 
> 
>