Re: [PATCH v9 34/37] hw/pci: Add helper to insert PCIe extended capability at a fixed offset

[Yi Liu <yi.l.liu@intel.com>]

On 2026/1/26 22:17, Shameer Kolothum wrote:
> Hi Yi,
> 

>>> +/*
>>> + * Insert a PCIe extended capability at a given offset.
>>> + *
>>> + * This helper only validates that the insertion does not overwrite an
>>> + * existing PCIe extended capability header, as corrupting a header would
>>> + * break the extended capability linked list.
>>> + *
>>> + * The caller must ensure that (offset, size) does not overlap with other
>>> + * registers or capability-specific register blocks. Overlaps with
>>> + * capability-specific registers are not checked and are considered a
>>> + * user-controlled override.
>>> + *
>>> + * Note: Best effort helper. The PCIe spec does not require extended
>>> + * capabilities to be ordered, but most devices use a forward-linked list.
>>> + * Devices that do not consistently use a forward-linked list may cause
>>> + * insertion to fail.
>>> + */
>>> +bool pcie_insert_capability(PCIDevice *dev, uint16_t cap_id, uint8_t
>> cap_ver,
>>> +                            uint16_t offset, uint16_t size)
>>> +{
>>> +    uint16_t pos = PCI_CONFIG_SPACE_SIZE, prev = 0;
>>> +    uint32_t header;
>>> +
>>> +    assert(pci_is_express(dev));
>>> +
>>> +    if (!QEMU_IS_ALIGNED(offset, PCI_EXT_CAP_ALIGN) ||
>>> +        size < 8 ||
>>> +        offset < PCI_CONFIG_SPACE_SIZE ||
>>> +        offset >= PCIE_CONFIG_SPACE_SIZE ||
>>> +        offset + size > PCIE_CONFIG_SPACE_SIZE) {
>>> +        return false;
>>> +    }
>>> +
>>> +    header = pci_get_long(dev->config + pos);
>>> +    if (!header) {
>>> +        /* No extended capability present, insertion must be at the ECAP head
>> */
>>> +        if (offset != pos) {
>>> +            return false;
>>> +        }
>>> +        pci_set_long(dev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
>>> +        goto out;
>>> +    }
>>> +
>>> +    while (header && pos && offset >= pos) {
>>> +        uint16_t next = PCI_EXT_CAP_NEXT(header);
>>> +
>>> +        /* Reject insertion inside an existing ECAP header (4 bytes) */
>>> +        if (offset < pos + PCI_EXT_CAP_ALIGN) {
>>> +            return false;
>>> +        }
>>
>> TBH. I was expecting to see a table that is similar with
>> pci_ext_cap_length[][1], and a helper to walk the ext cap list
>> to figure out the spared pos. But it might be over-enginering as
>> we rely more on user to give an offset that is for sure no conflict
>> with existing ecaps nor hidden registers. Could you also add a comment
>> in the code to note it?
>>
>> [1]
>> https://github.com/torvalds/linux/blob/63804fed149a6750ffd28610c5c1c9
>> 8cce6bd377/drivers/vfio/pci/vfio_pci_config.c#L71
>>
>> With the above nit, LGTM.
>>
>> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> 
> Thanks.
> 
> Yes, the helper depends on the caller to provide a suitable offset, since
> it's not generally possible to validate this due to hidden registers and
> device specific capability layouts, and this is already noted in the commit
> log and function comment.
> 
> If you had a specific wording in mind that would make this clearer, please
> let me know and I can incorporate it if this requires a respin.

no respin needed. Just noticed the comment in the function header is
enough. :)