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

[Alex Williamson]

On Wed, 2012-09-26 at 12:49 -0500, Matt Renzelmann wrote:
> > > 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:
> 
> I found a copy of the PCI-E 1.0 specification -- I don't have any later copy 
> available -- and it contains this text:
> 
> ============
> 7.9. PCI Express Extended Capabilities
> 
> PCI Express Extended Capability registers are located in device configuration 
> space at offsets 256 or greater ...
> 
> Each capability structure must be DWORD aligned.
> 
> ...
> 
> 7.9.3. PCI Express Enhanced Capability Header
> 
> Next Capability Offset – This field contains the
> offset to the next PCI Express capability structure or
> 000h if no other items exist in the linked list of
> capabilities.
> 
> ...
> 
> The bottom two bits of this offset are reserved and
> must be implemented as 00b although software
> must mask them to allow for future uses of these
> bits.
> ============
> 
> so, I think it's reasonable to align everything to 4-bytes all the time.  
> Here's another draft:
> 
> 
> static int pci_find_space(PCIDevice *pdev, uint32_t start, uint32_t size)
> {
>     int offset = start;
>     int i;
>     uint32_t *dword_used = &pdev->used[start];
> 
>     /* This approach ensures the capability is dword-aligned, as              
>                                           
>        required by the PCI and PCI-E specifications */
>     for (i = start; i < size; i += 4, dword_used++) {
>         if (*dword_used)
>             offset = i + 4;
>         else if (i - offset + 4 >= size)
>             return offset;
>     }

Mismatched uses of "size" here.  We need both the end of the range to
search and the size of the sub-range we're looking for.  Maybe start,
end, and size.  Thanks,

Alex

> 
>     return 0;
> }
> 
> static int pci_find_legacy_space(PCIDevice *pdev, uint8_t size) {
>     return pci_find_space(pdev, PCI_CONFIG_HEADER_SIZE, 
> PCI_CONFIG_SPACE_SIZE);
> }
> 
> static int pci_find_express_space(PCIDevice *pdev, uint16_t size) {
>     assert (pci_config_size(pdev) >= PCIE_CONFIG_SPACE_SIZE);
>     return pci_find_space(pdev, PCI_CONFIG_SPACE_SIZE, 
> PCIE_CONFIG_SPACE_SIZE);
> }
> 
>