> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: 14 January 2026 11:46
> To: Shameer Kolothum <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Jason Gunthorpe
> <[email protected]>; Nicolin Chen <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Nathan Chen
> <[email protected]>; Matt Ochs <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Krishnakant Jaju
> <[email protected]>; Michael S . Tsirkin <[email protected]>
> Subject: Re: [PATCH v7 33/36] hw/pci: Add helper to insert PCIe extended
> capability at a fixed offset
>
> External email: Use caution opening links or attachments
>
>
> On Sun, 11 Jan 2026 19:53:19 +0000
> Shameer Kolothum <[email protected]> wrote:
>
> > Add pcie_insert_capability(), a helper to insert a PCIe extended
> > capability into an existing extended capability list at a
> > caller-specified offset.
> >
> > Unlike pcie_add_capability(), which always appends a capability to the
> > end of the list, this helper preserves the existing list ordering while
> > allowing insertion at an arbitrary offset.
> >
> > The helper only validates that the insertion does not overwrite an
> > existing PCIe extended capability header, since corrupting a header
> > would break the extended capability linked list. Validation of overlaps
> > with other configuration space registers or capability-specific
> > register blocks is left to the caller.
> >
> > Cc: Michael S. Tsirkin <[email protected]>
> > Signed-off-by: Shameer Kolothum <[email protected]>
> Hi Shameer.
Happy new year!
>
> Random musings inline... Maybe I'm just failing in my spec grep skills.
>
> > ---
> > hw/pci/pcie.c | 58
> +++++++++++++++++++++++++++++++++++++++++++
> > include/hw/pci/pcie.h | 2 ++
> > 2 files changed, 60 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index b302de6419..8568a062a5 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -1050,6 +1050,64 @@ static void pcie_ext_cap_set_next(PCIDevice
> *dev, uint16_t pos, uint16_t next)
> > pci_set_long(dev->config + pos, header);
> > }
> >
> > +/*
> > + * 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.
> > + */
> > +bool pcie_insert_capability(PCIDevice *dev, uint16_t cap_id, uint8_t
> cap_ver,
> > + uint16_t offset, uint16_t size)
> > +{
> > + uint16_t prev = 0, next = 0;
> > + uint16_t cur = pci_get_word(dev->config + PCI_CONFIG_SPACE_SIZE);
> > +
> > + /* Walk the ext cap list to find insertion point */
> > + while (cur) {
> > + uint32_t hdr = pci_get_long(dev->config + cur);
> > + next = PCI_EXT_CAP_NEXT(hdr);
> > +
> > + /* Check we are not overwriting any existing CAP header area */
> > + if (offset >= cur && offset < cur + PCI_EXT_CAP_ALIGN) {
> > + return false;
> > + }
> > +
> > + prev = cur;
> > + cur = next;
> > + if (next == 0 || next > offset) {
>
> So this (sort of) relies on a thing I've never been able to find a clear
> statement of in the PCIe spec. Does Next Capability Offset have to be
> larger than the offset of the current record? I.e. Can we have
> backwards pointers?
That’s right. I also couldn’t find a place in the spec that explicitly
says the list must be forward only. A device doing a backward walk
would be pretty odd, hopefully nothing like that exists in the wild.
> Meh, I think this is fine, it just came up before and I couldn't find
> a reference to prove it!
>
> More importantly, if it isn't a requirement and a rare device turns up
> that has a backwards pointer, that just means there isn't a 'right'
> point in the list to put this at, so any random choice is fine and
> the next == 0 condition means we always fine an option.
Yes.
>
> > + break;
> > + }
> > + }
> > +
> > + /* Make sure, next CAP header area is not over written either */
>
> Looks like one space too few.
>
> > + if (next && (offset + size) >= next) {
> > + return false;
> > + }
> > +
> > + /* Insert new cap */
> > + pci_set_long(dev->config + offset,
> > + PCI_EXT_CAP(cap_id, cap_ver, cur));
> > + if (prev) {
> > + pcie_ext_cap_set_next(dev, prev, offset);
> > + } else {
> > + /* Insert at head (0x100) */
>
> Comment is a little confusing as you aren't inserting the new capability
> there. What I think this is actually doing is
>
> /*
> * Insert a Null Extended Capability (7.9.28 in the PCI 6.2 spec)
> * at head when there are no extended capabilities and use that to
> * point to the inserted capability at offset.
> */
Sure. However, Zhangfei has reported a crash with this and I have
reworked the logic a bit to cover few corner cases. Based on his
tests I will update this.
Thanks,
Shameer
> > + pci_set_word(dev->config + PCI_CONFIG_SPACE_SIZE, offset);
> > + }
> > +
> > + /* Make capability read-only by default */
> > + memset(dev->wmask + offset, 0, size);
> > + memset(dev->w1cmask + offset, 0, size);
> > + /* Check capability by default */
> > + memset(dev->cmask + offset, 0xFF, size);
> > + return true;
> > +}
>
