> -----Original Message-----
> From: Eric Auger <[email protected]>
> Sent: 19 January 2026 16:01
> To: Shameer Kolothum <[email protected]>; Zhangfei Gao
> <[email protected]>
> Cc: [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 1/14/26 11:36 AM, Shameer Kolothum wrote:
> > Hi Zhangfei,
> >
> >> -----Original Message-----
> >> From: Zhangfei Gao <[email protected]>
> >> Sent: 14 January 2026 04:18
> >> 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
> >>
> >>
> >> Hi, Shameer
> >>
> >> On Mon, 12 Jan 2026 at 03:58, 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]>
> >> The guest kernel fails to boot with para "ssidsize=16" with v7 series.
> >> Without ssidsize, guest kernel can boot no problem.
> > Thanks for giving this a spin.
> >
> >> However, pasid feature requires ssidsize.
> >> smmuv3_accel_get_viommu_flags
> >> if (s->ssidsize) {
> >> flags |= VIOMMU_FLAG_PASID_SUPPORTED;
> >>
> >> v6 does not has such issue, and does not requires ssidsize param.
> > As mentioned in cover letter this series has changed the way the overall
> > PASID is enabled. It now allows user to specify an offset to place the
> > PASID CAP for vfio-pci devices,
> >
> > -device vfio-pci,host=0018:06:00.0,..,x-vpasid-cap-offset=xxx
> >
> > If none is specified it will place it at the last offset as default.
> >
> > And you need "ssidsize" to specify the SMMUv3 sub stream Id bits.
> >
> >> log:
> >> ASSERT_EFI_ERROR (Status = Invalid Parameter) ASSERT [PciBusDxe]
> >>
> /home/linaro/work/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c(626):
> >> !(((INTN)(RETURN_STATUS)(Status)) < 0)
> > It could be that the pcie_insert_capability() helper added here is
> > corrupting
> > the config space. Looking at it again, I can see that it is not handling a
> > few
> > corner cases. Could you please replace the pcie_insert_capability() with
> > the below and retest.
> >
> > Thanks,
> > Shameer
> >
> > ...
> >
> > bool pcie_insert_capability(PCIDevice *dev, uint16_t cap_id, uint8_t
> > cap_ver,
> > uint16_t offset, uint16_t size)
> > {
> > uint16_t pos = PCI_CONFIG_SPACE_SIZE, prev = 0;
> > uint32_t header;
> >
> > assert(offset >= PCI_CONFIG_SPACE_SIZE);
> > assert(offset < (uint16_t)(offset + size));
> > assert((uint16_t)(offset + size) <= PCIE_CONFIG_SPACE_SIZE);
> > assert(size >= 8);
> > assert(pci_is_express(dev));
> >
> > header = pci_get_long(dev->config + pos);
> > if (!header) {
> > /* No extended capability, check requested offset is at
> PCI_CONFIG_SPACE_SIZE*/
> > if (offset != pos) {
> > return false;
> > }
> > pci_set_long(dev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
> > goto out;
> > }
> >
> > while (header && pos && offset >= pos) {
> > uint16_t next = PCI_EXT_CAP_NEXT(header);
> >
> > /* Reject insertion inside an existing ECAP header (4 bytes) */
> If I understand correctly you don't check the whole EXT CAP size but
> just the header, correct?
Yes. That’s correct.
> > if (offset < pos + PCI_EXT_CAP_ALIGN) {
> > return false;
> > }
> >
> > prev = pos;
> > pos = next;
> > header = pos ? pci_get_long(dev->config + pos) : 0;
> > }
> >
> > pci_set_long(dev->config + offset, PCI_EXT_CAP(cap_id, cap_ver, pos));
> > if (prev) {
> > pcie_ext_cap_set_next(dev, prev, offset);
> > }
> >
> > out:
> > /* 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;
> > }
> Besides
>
> Reviewed-by: Eric Auger <[email protected]>
Thanks,
Shameer
