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. 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? 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. > + 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. */ > + 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; > +}
