Re: [PATCH v7 33/36] hw/pci: Add helper to insert PCIe extended capability at a fixed offset

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jan 14, 2026 at 12:26:29PM +0000, Shameer Kolothum wrote:
> 
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Sent: 14 January 2026 11:46
> > To: Shameer Kolothum <skolothumtho@nvidia.com>
> > Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; Jason Gunthorpe
> > <jgg@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; ddutile@redhat.com;
> > berrange@redhat.com; clg@redhat.com; alex@shazbot.org; Nathan Chen
> > <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> > smostafa@google.com; wangzhou1@hisilicon.com;
> > jiangkunkun@huawei.com; zhangfei.gao@linaro.org;
> > zhenzhong.duan@intel.com; yi.l.liu@intel.com; Krishnakant Jaju
> > <kjaju@nvidia.com>; Michael S . Tsirkin <mst@redhat.com>
> > 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 <skolothumtho@nvidia.com> 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 <mst@redhat.com>
> > > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> > 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;
> > > +}
> > 
>