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

[Don Slutz <Don@CloudSwitch.Com>]

On 09/26/12 13:55, Alex Williamson wrote:
> 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)
I would think that dword_used needs to be based on i not start.
>>              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);
>> }
>>
>>
>
>
>
    -Don Slutz