Re: [RFC 7/8] hw/pci: hw/cxl: Wire SVC initialization into port realize functions.

[Shrihari E S <shrihari.s@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3536 bytes --]

On 17/06/26 11:45PM, Junjie Cao wrote:
>Hi Shrihari,
>
>On Tue,  9 Jun 2026 16:28:35 +0530, Shrihari E S wrote:
>> +    pcie_config_uio_svc(d, errp);
>>      pcie_cap_arifwd_init(d);
>>      pcie_cap_deverr_init(d);
>>      pcie_cap_slot_init(d, s);
>
>In rp_realize() (and the two xio3130 ports) pcie_config_uio_svc() runs
>before pcie_aer_init().  pcie_svc_cap_init() falls back to offset 0x100
>when no extended capability exists yet, so SVC is placed at 0x100 at
>this point -- and then pcie_aer_init() puts AER at the same 0x100 and
>overwrites it, while dev->exp.svc_cap still points there.
>
>The visible effect is that "-device pcie-root-port,x-uio-svc=on" comes
>up with no SVC capability in the extended config space (the chain is
>just AER -> ACS), and no error is reported, so the request is silently
>dropped.  I reproduced this by walking the extended capability list of
>the root port over ECAM: SVC (0x35) never appears, even though the
>property was set.
>
>> +    rc = pcie_config_uio_svc(pci_dev, errp);
>> +    if (p->flitmode && rc >= 0) {
>> +        crp->uio_capable = true;
>> +    }
>
>On cxl-rp this runs a second time: cxl_rp_realize() first calls the
>parent rp_realize() (which already calls pcie_config_uio_svc() per the
>hunk above) and then calls it again directly.  The first call collides
>with AER at 0x100 as above; the second runs after the extended caps
>exist, so SVC ends up correctly at 0x200.  So cxl-rp happens to work,
>but via a double init.
>
>Just moving the call to after pcie_aer_init() isn't quite enough on
>its own: the CXL ports (cxl-rp/usp/dsp) build their DVSECs at fixed
>offsets and then add SVC themselves at the end, so if the shared
>parent rp_realize() also adds SVC before those DVSECs are built, the
>cxl-rp ends up with its DVSECs off the chain.
>
>What worked for me was to move the SVC call after the ECAP inits in
>the pure-PCIe paths and skip it in the parent when the device is CXL,
>letting the CXL realize functions keep adding it last as they already
>do:
>
>  - rp_realize(): move pcie_config_uio_svc() to after pcie_acs_init(),
>    guarded by if (!pci_is_cxl(d)) so cxl-rp's own call (which runs
>    after build_dvsecs()) remains the one that places SVC.
>  - xio3130_upstream/downstream realize(): move the call to after
>    pcie_aer_init().
>  - cxl-rp/usp/dsp: unchanged, they already add it last.
>
>With that, plain pcie-root-port, the xio3130 switch ports and the CXL
>ports all come up with SVC at 0x200, and cxl-rp keeps its four DVSECs.
>

Hi Junjie,

     Thank you for pointing this bug, got missed in our testing. Will check
     and move the svc_init() function properly in the next version of the patch.

>One thing to note is that this only gets the capability enumerated --
>I haven't verified the SVC behaviour beyond that.  Looking at
>pcie_svc_cap_write_config()/pcie_svc_apply_gating(), writing the SVC
>Enable bit just copies ctrl/status into the shadow struct; no VC
>Resource Status bit (the per-VC negotiation bit, SVC_VC_NEGO) is ever
>set, and the SVC Status register stays zero.  So a guest that enables
>SVC and polls for negotiation wouldn't see anything change.  If the
>data-plane series on top is expected to drive that, fine -- I just
>wanted to flag that as things stand the enable is observationally a
>no-op, in case a driver is expected to key off it.
>
>Many thanks,
>Junjie

Yeah this patch series is meant for enumeration (probe) puprpose only
and yes we have plan to add data plane support as well.

Thanks,
Shrihari


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]