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

[Don Slutz <Don@CloudSwitch.Com>]

On 09/26/12 14:02, Don Slutz wrote:
> 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++) {
Opps, I missed the dword_used++

Forget this comment...

>>>          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
>
   -Don Slutz