On Wed, 14 Jan 2026 07:35:01 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
> 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. I think best we can do here is a comment saying this is 'best effort' attempt to place it based on many devices using increasing addresses. (I can't claim to have seen any that don't, but I've only looked a few dozen of my career :) Jonathan
