On Wed, Jan 14, 2026 at 12:26:29PM +0000, Shameer Kolothum wrote:
>
>
> > -----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.
Yes, there's no reason not to have such pointers, with either
PCIe or classical PCI capability.
> > 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;
> > > +}
> >
>
