Re: [PATCH 1/3] xio3130_downstream: Add ACS support for downstream PCIe ports

[Tushar Dave <tdave@nvidia.com>]

On 10/2/2026 5:38 AM, Shameer Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Tushar Dave <tdave@nvidia.com>
>> Sent: 22 September 2026 22:50
>> To: qemu-devel@nongnu.org
>> Cc: mst@redhat.com; philmd@oss.qualcomm.com; zhao1.liu@intel.com;
>> Yishai Hadas <yishaih@nvidia.com>; Alex Williamson
>> <alwilliamson@nvidia.com>; Jason Gunthorpe <jgg@nvidia.com>; Shameer
>> Kolothum Thodi <skolothumtho@nvidia.com>
>> Subject: [PATCH 1/3] xio3130_downstream: Add ACS support for
>> downstream PCIe ports
>>
>> When a PASID-capable device is added behind the PCIe downstream port,
>> for example, an Nvidia GPU, the PCIe downstream ports must expose ACS
>> capability otherwise PASID won't get enabled.
>>
>> In addition, the other use case is GPUDirect RDMA using Data Direct
>> that must require special ACS controls at the PCIe downstream ports
>> for P2P communication.
>>
>> Gate it behind a new x-pcie-acs property (default on), following the
> 
> Why default on? It only matters for the use cases above. With it on,
> a guest with a vIOMMU behind an xio3130 downstream port may see its
> IOMMU groups change. Is that intended? If so, please mention it in the
> commit message.

'x-pcie-acs' exists only for migration compatibility. 'x-pcie-acs' does
not control any ACS control bits. It defaults on so that new machine
types get the capability without an extra option, and older machine
types turn it off through the hw_compat entry, like other properties do
today, so migration is unchanged.

> 
>> x-pcie-* compat-bit convention (x-pcie-ext-tag/x-pcie-err-unc-mask/
>> x-pcie-ari-nextfn-1), so older machine types can disable it via
>> hw_compat to preserve migration compatibility.
>>
>> Signed-off-by: Tushar Dave <tdave@nvidia.com>
>> ---
>>  hw/core/machine.c                  | 1 +
>>  hw/pci-bridge/xio3130_downstream.c | 7 +++++++
>>  hw/pci/pci.c                       | 2 ++
>>  include/hw/pci/pci.h               | 3 +++
>>  4 files changed, 13 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 92f7be52a1..4fd48f594f 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -43,6 +43,7 @@
>>  GlobalProperty hw_compat_11_1[] = {
>>      { "sysbus-ehci-usb", "x-migrate-fetch-addr-64bit", "off" },
>>      { "pci-ehci-usb", "x-migrate-fetch-addr-64bit", "off" },
>> +    { TYPE_PCI_DEVICE, "x-pcie-acs", "off" }, /* Added in 11.2 */
>>  };
>>  const size_t hw_compat_11_1_len = G_N_ELEMENTS(hw_compat_11_1);
>>
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-
>> bridge/xio3130_downstream.c
>> index 0c3fed3053..87ba30eb36 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -40,6 +40,8 @@
>>  #define XIO3130_SSVID_SSID              0
>>  #define XIO3130_EXP_OFFSET              0x90
>>  #define XIO3130_AER_OFFSET              0x100
>> +#define XIO3130_ACS_OFFSET \
>> +    (XIO3130_AER_OFFSET + PCI_ERR_SIZEOF)
>>
>>  static void xio3130_downstream_write_config(PCIDevice *d, uint32_t
>> address,
>>                                           uint32_t val, int len)
>> @@ -60,6 +62,7 @@ static void xio3130_downstream_reset(DeviceState
>> *qdev)
>>      pcie_cap_deverr_reset(d);
>>      pcie_cap_slot_reset(d);
>>      pcie_cap_arifwd_reset(d);
>> +    pcie_acs_reset(d);
>>      pci_bridge_reset(qdev);
>>  }
>>
>> @@ -111,6 +114,10 @@ static void xio3130_downstream_realize(PCIDevice
>> *d, Error **errp)
>>          goto err;
>>      }
>>
>> +    if (d->cap_present & QEMU_PCIE_CAP_ACS) {
>> +        pcie_acs_init(d, XIO3130_ACS_OFFSET);
>> +    }
>> +
>>      return;
>>
>>  err:
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 0efb4eb4bb..807729ca75 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -105,6 +105,8 @@ static const Property pci_props[] = {
>>      DEFINE_PROP_STRING("sriov-pf", PCIDevice, sriov_pf),
>>      DEFINE_PROP_BIT("x-pcie-ext-tag", PCIDevice, cap_present,
>>                      QEMU_PCIE_EXT_TAG_BITNR, true),
>> +    DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
>> +                    QEMU_PCIE_ACS_BITNR, true),
> 
> This is defined on every PCI device but only xio3130 downstream reads
> it.  I think it is better to limit the scope.

Sure thing.
> 
> Thanks,
> Shameer

Thanks.
-Tushar