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

["Matt Renzelmann" <mjr@cs.wisc.edu>]

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

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