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?
> 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]>
Eric
