[PATCH v2 0/3] pcie/acs: configurable ACS capabilities for root and downstream ports

[PATCH v2 0/3] pcie/acs: configurable ACS capabilities for root and downstream ports

[Tushar Dave]

This series adds an 'acs-ctrl' property to PCIe root ports and downstream
ports, letting the hypervisor expose a specific PCIe ACS (Access Control
Services) configuration to the guest instead of QEMU always advertising
every ACS capability the port structurally supports.

This matters for P2P (peer-to-peer) topologies such as GPUDirect RDMA,
where the actual ACS configuration in effect must match what the host
side is doing -- otherwise the guest sees a topology/configuration that
doesn't reflect reality, which is undefined behavior and can fail either
in software or in hardware.

Patch 1 adds the missing ACS capability to xio3130 downstream ports
(previously only root ports exposed one), gated behind a new
'x-pcie-acs' property so older machine types can disable it via
hw_compat and preserve migration compatibility.

Patch 2 introduces the 'acs-ctrl' property itself on both root and
downstream ports, so the configured value -- not just the maximal
capability set -- is what gets exposed to the guest.

Patch 3 makes the ACS Control register read-only to the guest once
'acs-ctrl' has been explicitly configured, so a guest OS doing normal
PCIe programming can't silently override the host-configured ACS
state. An explicitly configured value of 0 is treated the same as any
other configured value (locked), distinct from the property never
being set at all (which stays guest-writable, unchanged from current
behavior).

Changes in v2:
- Patch 1: Limit x-pcie-acs to the xio3130 downstream port
- Patch 2: Error out when acs-ctrl is set on a port without ACS, reword
  the commit message and the unsupported-bits error
- Patch 3: Make pcie_acs_init() safe for devices that are not PCIe
  ports, clarify the acs-ctrl property description

Link to v1: https://lore.kernel.org/qemu-devel/20260922215008.2554402-1-tdave@nvidia.com/

Tushar Dave (2):
  xio3130_downstream: Add ACS support for downstream PCIe ports
  hw/pci: Make ACS Control read-only to guests

Yishai Hadas (1):
  hw/pci: Expose ACS ctrl on Guest as configured by the hypervisor

 hw/core/machine.c                  |  1 +
 hw/pci-bridge/pcie_root_port.c     | 14 ++++++-
 hw/pci-bridge/xio3130_downstream.c | 19 ++++++++++
 hw/pci/pcie.c                      | 61 ++++++++++++++++++++++++++++--
 include/hw/pci/pci.h               |  3 ++
 include/hw/pci/pcie.h              |  8 +++-
 include/hw/pci/pcie_port.h         |  2 +
 7 files changed, 100 insertions(+), 8 deletions(-)

-- 
2.34.1

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

[Tushar Dave]

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
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 | 9 +++++++++
 include/hw/pci/pci.h               | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d9ac4b419a..b1b3911430 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" },
+    { "xio3130-downstream", "x-pcie-acs", "off" },
 };
 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..6a9df5c174 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:
@@ -137,6 +144,8 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
 static const Property xio3130_downstream_props[] = {
     DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
+    DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
+                    QEMU_PCIE_ACS_BITNR, true),
 };
 
 static const VMStateDescription vmstate_xio3130_downstream = {
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index f2448e941a..dd52b9639e 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -231,6 +231,9 @@ enum {
     QEMU_PCI_CAP_PM = (1 << QEMU_PCI_CAP_PM_BITNR),
 #define QEMU_PCI_SKIP_RESET_ON_CPR_BITNR 15
     QEMU_PCI_SKIP_RESET_ON_CPR = (1 << QEMU_PCI_SKIP_RESET_ON_CPR_BITNR),
+    /* PCIe ACS (Access Control Services) extended capability present */
+#define QEMU_PCIE_ACS_BITNR 16
+    QEMU_PCIE_CAP_ACS = (1 << QEMU_PCIE_ACS_BITNR),
 };
 
 typedef struct PCIINTxRoute {
-- 
2.34.1

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

[Michael S. Tsirkin]

On Mon, Oct 05, 2026 at 08:31:31PM -0500, Tushar Dave wrote:
> 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
> 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>

Does TI X3130 have ACS? If not will the purpose be
served better by the new generic port recently proposed?

> ---
>  hw/core/machine.c                  | 1 +
>  hw/pci-bridge/xio3130_downstream.c | 9 +++++++++
>  include/hw/pci/pci.h               | 3 +++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d9ac4b419a..b1b3911430 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" },
> +    { "xio3130-downstream", "x-pcie-acs", "off" },
>  };
>  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..6a9df5c174 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:
> @@ -137,6 +144,8 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
>  static const Property xio3130_downstream_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> +    DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
> +                    QEMU_PCIE_ACS_BITNR, true),
>  };
>  
>  static const VMStateDescription vmstate_xio3130_downstream = {
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index f2448e941a..dd52b9639e 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -231,6 +231,9 @@ enum {
>      QEMU_PCI_CAP_PM = (1 << QEMU_PCI_CAP_PM_BITNR),
>  #define QEMU_PCI_SKIP_RESET_ON_CPR_BITNR 15
>      QEMU_PCI_SKIP_RESET_ON_CPR = (1 << QEMU_PCI_SKIP_RESET_ON_CPR_BITNR),
> +    /* PCIe ACS (Access Control Services) extended capability present */
> +#define QEMU_PCIE_ACS_BITNR 16
> +    QEMU_PCIE_CAP_ACS = (1 << QEMU_PCIE_ACS_BITNR),
>  };
>  
>  typedef struct PCIINTxRoute {
> -- 
> 2.34.1

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

[Tushar Dave]

On 10/5/2026 11:49 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 05, 2026 at 08:31:31PM -0500, Tushar Dave wrote:
>> 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
>> 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>
> 
> Does TI X3130 have ACS? If not will the purpose be
> served better by the new generic port recently proposed?


As per my search, the physical TI XIO3130 does not implement ACS.

I am not aware if the capabilities of the emulated XIO3130 are expected
to exactly match those of the physical XIO3130. My assumption was that
the emulated device can provide the PCIe capabilities needed to
construct and configure the virtual topology, even if those capabilities
are not present in the physical XIO3130.

I added ACS to the XIO3130 devices because they are currently used to
build PCIe topologies. I have not looked into the generic PCIe port
support in detail yet, so I can't comment on whether it would be a
better fit for these existing PCIe topology use cases.

Thanks.
-Tushar


> 
>> ---
>>  hw/core/machine.c                  | 1 +
>>  hw/pci-bridge/xio3130_downstream.c | 9 +++++++++
>>  include/hw/pci/pci.h               | 3 +++
>>  3 files changed, 13 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index d9ac4b419a..b1b3911430 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" },
>> +    { "xio3130-downstream", "x-pcie-acs", "off" },
>>  };
>>  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..6a9df5c174 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:
>> @@ -137,6 +144,8 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
>>  static const Property xio3130_downstream_props[] = {
>>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> +    DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
>> +                    QEMU_PCIE_ACS_BITNR, true),
>>  };
>>  
>>  static const VMStateDescription vmstate_xio3130_downstream = {
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index f2448e941a..dd52b9639e 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -231,6 +231,9 @@ enum {
>>      QEMU_PCI_CAP_PM = (1 << QEMU_PCI_CAP_PM_BITNR),
>>  #define QEMU_PCI_SKIP_RESET_ON_CPR_BITNR 15
>>      QEMU_PCI_SKIP_RESET_ON_CPR = (1 << QEMU_PCI_SKIP_RESET_ON_CPR_BITNR),
>> +    /* PCIe ACS (Access Control Services) extended capability present */
>> +#define QEMU_PCIE_ACS_BITNR 16
>> +    QEMU_PCIE_CAP_ACS = (1 << QEMU_PCIE_ACS_BITNR),
>>  };
>>  
>>  typedef struct PCIINTxRoute {
>> -- 
>> 2.34.1
> 

[PATCH v2 2/3] hw/pci: Expose ACS ctrl on Guest as configured by the hypervisor

[Tushar Dave]

From: Yishai Hadas <yishaih@nvidia.com>

To support P2P on Guest we must expose to the guest OS the actual
PCIe topology and configuration as set by the hypervisor. The ACS
control must be set accordingly to have functional P2P. A mismatch
between the hypervisor's ACS configuration and the guest-visible ACS
Control register can prevent P2P between devices from working as
expected.

Extend both root port and downstream port to accept ACS Control bits
that match the hypervisor configuration and expose them to the guest.

Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Co-developed-by: Tushar Dave <tdave@nvidia.com>
Signed-off-by: Tushar Dave <tdave@nvidia.com>
---
 hw/pci-bridge/pcie_root_port.c     | 13 +++++++++++--
 hw/pci-bridge/xio3130_downstream.c | 13 +++++++++++--
 hw/pci/pcie.c                      | 17 ++++++++++++++---
 include/hw/pci/pcie.h              |  5 +++--
 include/hw/pci/pcie_port.h         |  1 +
 5 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 7c3e78010b..e7cce8c0ee 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -46,6 +46,7 @@ static void rp_write_config(PCIDevice *d, uint32_t address,
 static void rp_reset_hold(Object *obj, ResetType type)
 {
     PCIDevice *d = PCI_DEVICE(obj);
+    PCIEPort *p = PCIE_PORT(d);
     DeviceState *qdev = DEVICE(obj);
 
     rp_aer_vector_update(d);
@@ -53,7 +54,7 @@ static void rp_reset_hold(Object *obj, ResetType type)
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
     pcie_cap_arifwd_reset(d);
-    pcie_acs_reset(d);
+    pcie_acs_reset(d, p->acs_ctrl);
     pcie_aer_root_reset(d);
     pci_bridge_reset(qdev);
     pci_bridge_disable_base_limit(d);
@@ -118,11 +119,18 @@ static void rp_realize(PCIDevice *d, Error **errp)
     rp_aer_vector_update(d);
 
     if (rpc->acs_offset) {
-        pcie_acs_init(d, rpc->acs_offset);
+        rc = pcie_acs_init(d, rpc->acs_offset, p->acs_ctrl, errp);
+        if (rc < 0) {
+            goto err;
+        }
+    } else if (p->acs_ctrl) {
+        error_setg(errp, "acs-ctrl is not supported by this root port");
+        goto err;
     }
     return;
 
 err:
+    pcie_aer_exit(d);
     pcie_chassis_del_slot(s);
 err_pcie_cap:
     pcie_cap_exit(d);
@@ -151,6 +159,7 @@ static void rp_exit(PCIDevice *d)
 static const Property rp_props[] = {
     DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
+    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
 };
 
 static void rp_instance_post_init(Object *obj)
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 6a9df5c174..e243dfdf7d 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -58,11 +58,12 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
 static void xio3130_downstream_reset(DeviceState *qdev)
 {
     PCIDevice *d = PCI_DEVICE(qdev);
+    PCIEPort *p = PCIE_PORT(d);
 
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
     pcie_cap_arifwd_reset(d);
-    pcie_acs_reset(d);
+    pcie_acs_reset(d, p->acs_ctrl);
     pci_bridge_reset(qdev);
 }
 
@@ -115,12 +116,19 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
     }
 
     if (d->cap_present & QEMU_PCIE_CAP_ACS) {
-        pcie_acs_init(d, XIO3130_ACS_OFFSET);
+        rc = pcie_acs_init(d, XIO3130_ACS_OFFSET, p->acs_ctrl, errp);
+        if (rc < 0) {
+            goto err;
+        }
+    } else if (p->acs_ctrl) {
+        error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
+        goto err;
     }
 
     return;
 
 err:
+    pcie_aer_exit(d);
     pcie_chassis_del_slot(s);
 err_pcie_cap:
     pcie_cap_exit(d);
@@ -146,6 +154,7 @@ static const Property xio3130_downstream_props[] = {
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
     DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
                     QEMU_PCIE_ACS_BITNR, true),
+    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
 };
 
 static const VMStateDescription vmstate_xio3130_downstream = {
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 42bcb9206d..af24ec7fd9 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -1260,7 +1260,8 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned)
 }
 
 /* ACS (Access Control Services) */
-void pcie_acs_init(PCIDevice *dev, uint16_t offset)
+int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
+                  Error **errp)
 {
     bool is_downstream = pci_is_express_downstream_port(dev);
     uint16_t cap_bits = 0;
@@ -1284,16 +1285,26 @@ void pcie_acs_init(PCIDevice *dev, uint16_t offset)
          */
         cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
             PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
+
+        if (ctrl_bits & ~cap_bits) {
+            error_setg(errp,
+                       "acs-ctrl 0x%hx has bits not supported by this port "
+                       "(supported: 0x%hx)", ctrl_bits, cap_bits);
+            return -EINVAL;
+        }
     }
 
     pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
     pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
+    pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
+
+    return 0;
 }
 
-void pcie_acs_reset(PCIDevice *dev)
+void pcie_acs_reset(PCIDevice *dev, uint16_t val)
 {
     if (dev->exp.acs_cap) {
-        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
+        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, val);
     }
 }
 
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index ec25e7a7de..678ad73f7f 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -137,8 +137,9 @@ bool pcie_insert_capability(PCIDevice *dev, uint16_t cap_id, uint8_t cap_ver,
                             uint16_t offset, uint16_t size);
 void pcie_sync_bridge_lnk(PCIDevice *dev);
 
-void pcie_acs_init(PCIDevice *dev, uint16_t offset);
-void pcie_acs_reset(PCIDevice *dev);
+int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
+                  Error **errp);
+void pcie_acs_reset(PCIDevice *dev, uint16_t val);
 
 void pcie_ari_init(PCIDevice *dev, uint16_t offset);
 void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
index b28af067a6..2ad9a563d9 100644
--- a/include/hw/pci/pcie_port.h
+++ b/include/hw/pci/pcie_port.h
@@ -36,6 +36,7 @@ struct PCIEPort {
 
     /* pci express switch port */
     uint8_t     port;
+    uint16_t    acs_ctrl;
 };
 
 void pcie_port_init_reg(PCIDevice *d);
-- 
2.34.1

Re: [PATCH v2 2/3] hw/pci: Expose ACS ctrl on Guest as configured by the hypervisor

[Michael S. Tsirkin]

On Mon, Oct 05, 2026 at 08:31:32PM -0500, Tushar Dave wrote:
> From: Yishai Hadas <yishaih@nvidia.com>
> 
> To support P2P on Guest we must expose to the guest OS the actual
> PCIe topology and configuration as set by the hypervisor. The ACS
> control must be set accordingly to have functional P2P. A mismatch
> between the hypervisor's ACS configuration and the guest-visible ACS
> Control register can prevent P2P between devices from working as
> expected.
> 
> Extend both root port and downstream port to accept ACS Control bits
> that match the hypervisor configuration and expose them to the guest.
> 
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> Co-developed-by: Tushar Dave <tdave@nvidia.com>
> Signed-off-by: Tushar Dave <tdave@nvidia.com>
> ---
>  hw/pci-bridge/pcie_root_port.c     | 13 +++++++++++--
>  hw/pci-bridge/xio3130_downstream.c | 13 +++++++++++--
>  hw/pci/pcie.c                      | 17 ++++++++++++++---
>  include/hw/pci/pcie.h              |  5 +++--
>  include/hw/pci/pcie_port.h         |  1 +
>  5 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 7c3e78010b..e7cce8c0ee 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -46,6 +46,7 @@ static void rp_write_config(PCIDevice *d, uint32_t address,
>  static void rp_reset_hold(Object *obj, ResetType type)
>  {
>      PCIDevice *d = PCI_DEVICE(obj);
> +    PCIEPort *p = PCIE_PORT(d);
>      DeviceState *qdev = DEVICE(obj);
>  
>      rp_aer_vector_update(d);
> @@ -53,7 +54,7 @@ static void rp_reset_hold(Object *obj, ResetType type)
>      pcie_cap_deverr_reset(d);
>      pcie_cap_slot_reset(d);
>      pcie_cap_arifwd_reset(d);
> -    pcie_acs_reset(d);
> +    pcie_acs_reset(d, p->acs_ctrl);
>      pcie_aer_root_reset(d);
>      pci_bridge_reset(qdev);
>      pci_bridge_disable_base_limit(d);
> @@ -118,11 +119,18 @@ static void rp_realize(PCIDevice *d, Error **errp)
>      rp_aer_vector_update(d);
>  
>      if (rpc->acs_offset) {
> -        pcie_acs_init(d, rpc->acs_offset);
> +        rc = pcie_acs_init(d, rpc->acs_offset, p->acs_ctrl, errp);
> +        if (rc < 0) {
> +            goto err;
> +        }
> +    } else if (p->acs_ctrl) {
> +        error_setg(errp, "acs-ctrl is not supported by this root port");
> +        goto err;
>      }
>      return;
>  
>  err:
> +    pcie_aer_exit(d);
>      pcie_chassis_del_slot(s);
>  err_pcie_cap:
>      pcie_cap_exit(d);
> @@ -151,6 +159,7 @@ static void rp_exit(PCIDevice *d)
>  static const Property rp_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> +    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
>  };
>  
>  static void rp_instance_post_init(Object *obj)
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index 6a9df5c174..e243dfdf7d 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -58,11 +58,12 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
>  static void xio3130_downstream_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = PCI_DEVICE(qdev);
> +    PCIEPort *p = PCIE_PORT(d);
>  
>      pcie_cap_deverr_reset(d);
>      pcie_cap_slot_reset(d);
>      pcie_cap_arifwd_reset(d);
> -    pcie_acs_reset(d);
> +    pcie_acs_reset(d, p->acs_ctrl);
>      pci_bridge_reset(qdev);
>  }
>  
> @@ -115,12 +116,19 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>      }
>  
>      if (d->cap_present & QEMU_PCIE_CAP_ACS) {
> -        pcie_acs_init(d, XIO3130_ACS_OFFSET);
> +        rc = pcie_acs_init(d, XIO3130_ACS_OFFSET, p->acs_ctrl, errp);
> +        if (rc < 0) {
> +            goto err;
> +        }
> +    } else if (p->acs_ctrl) {
> +        error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
> +        goto err;

Tying a stable QAPI to an unstable one like this is not really
reasonable.

>      }
>  
>      return;
>  
>  err:
> +    pcie_aer_exit(d);
>      pcie_chassis_del_slot(s);
>  err_pcie_cap:
>      pcie_cap_exit(d);
> @@ -146,6 +154,7 @@ static const Property xio3130_downstream_props[] = {
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>      DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
>                      QEMU_PCIE_ACS_BITNR, true),
> +    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
>  };
>  
>  static const VMStateDescription vmstate_xio3130_downstream = {
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 42bcb9206d..af24ec7fd9 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -1260,7 +1260,8 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned)
>  }
>  
>  /* ACS (Access Control Services) */
> -void pcie_acs_init(PCIDevice *dev, uint16_t offset)
> +int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
> +                  Error **errp)
>  {
>      bool is_downstream = pci_is_express_downstream_port(dev);
>      uint16_t cap_bits = 0;
> @@ -1284,16 +1285,26 @@ void pcie_acs_init(PCIDevice *dev, uint16_t offset)
>           */
>          cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
>              PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
> +
> +        if (ctrl_bits & ~cap_bits) {
> +            error_setg(errp,
> +                       "acs-ctrl 0x%hx has bits not supported by this port "
> +                       "(supported: 0x%hx)", ctrl_bits, cap_bits);
> +            return -EINVAL;
> +        }
>      }
>  
>      pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
>      pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> +    pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
> +
> +    return 0;
>  }
>  
> -void pcie_acs_reset(PCIDevice *dev)
> +void pcie_acs_reset(PCIDevice *dev, uint16_t val)
>  {
>      if (dev->exp.acs_cap) {
> -        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
> +        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, val);
>      }
>  }
>  
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index ec25e7a7de..678ad73f7f 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -137,8 +137,9 @@ bool pcie_insert_capability(PCIDevice *dev, uint16_t cap_id, uint8_t cap_ver,
>                              uint16_t offset, uint16_t size);
>  void pcie_sync_bridge_lnk(PCIDevice *dev);
>  
> -void pcie_acs_init(PCIDevice *dev, uint16_t offset);
> -void pcie_acs_reset(PCIDevice *dev);
> +int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
> +                  Error **errp);
> +void pcie_acs_reset(PCIDevice *dev, uint16_t val);
>  
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset);
>  void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> index b28af067a6..2ad9a563d9 100644
> --- a/include/hw/pci/pcie_port.h
> +++ b/include/hw/pci/pcie_port.h
> @@ -36,6 +36,7 @@ struct PCIEPort {
>  
>      /* pci express switch port */
>      uint8_t     port;
> +    uint16_t    acs_ctrl;
>  };
>  
>  void pcie_port_init_reg(PCIDevice *d);
> -- 
> 2.34.1

Re: [PATCH v2 2/3] hw/pci: Expose ACS ctrl on Guest as configured by the hypervisor

[Tushar Dave]

On 10/5/2026 11:45 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 05, 2026 at 08:31:32PM -0500, Tushar Dave wrote:
>> From: Yishai Hadas <yishaih@nvidia.com>
>>
>> To support P2P on Guest we must expose to the guest OS the actual
>> PCIe topology and configuration as set by the hypervisor. The ACS
>> control must be set accordingly to have functional P2P. A mismatch
>> between the hypervisor's ACS configuration and the guest-visible ACS
>> Control register can prevent P2P between devices from working as
>> expected.
>>
>> Extend both root port and downstream port to accept ACS Control bits
>> that match the hypervisor configuration and expose them to the guest.
>>
>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>> Co-developed-by: Tushar Dave <tdave@nvidia.com>
>> Signed-off-by: Tushar Dave <tdave@nvidia.com>
>> ---
>>  hw/pci-bridge/pcie_root_port.c     | 13 +++++++++++--
>>  hw/pci-bridge/xio3130_downstream.c | 13 +++++++++++--
>>  hw/pci/pcie.c                      | 17 ++++++++++++++---
>>  include/hw/pci/pcie.h              |  5 +++--
>>  include/hw/pci/pcie_port.h         |  1 +
>>  5 files changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> index 7c3e78010b..e7cce8c0ee 100644
>> --- a/hw/pci-bridge/pcie_root_port.c
>> +++ b/hw/pci-bridge/pcie_root_port.c
>> @@ -46,6 +46,7 @@ static void rp_write_config(PCIDevice *d, uint32_t address,
>>  static void rp_reset_hold(Object *obj, ResetType type)
>>  {
>>      PCIDevice *d = PCI_DEVICE(obj);
>> +    PCIEPort *p = PCIE_PORT(d);
>>      DeviceState *qdev = DEVICE(obj);
>>  
>>      rp_aer_vector_update(d);
>> @@ -53,7 +54,7 @@ static void rp_reset_hold(Object *obj, ResetType type)
>>      pcie_cap_deverr_reset(d);
>>      pcie_cap_slot_reset(d);
>>      pcie_cap_arifwd_reset(d);
>> -    pcie_acs_reset(d);
>> +    pcie_acs_reset(d, p->acs_ctrl);
>>      pcie_aer_root_reset(d);
>>      pci_bridge_reset(qdev);
>>      pci_bridge_disable_base_limit(d);
>> @@ -118,11 +119,18 @@ static void rp_realize(PCIDevice *d, Error **errp)
>>      rp_aer_vector_update(d);
>>  
>>      if (rpc->acs_offset) {
>> -        pcie_acs_init(d, rpc->acs_offset);
>> +        rc = pcie_acs_init(d, rpc->acs_offset, p->acs_ctrl, errp);
>> +        if (rc < 0) {
>> +            goto err;
>> +        }
>> +    } else if (p->acs_ctrl) {
>> +        error_setg(errp, "acs-ctrl is not supported by this root port");
>> +        goto err;
>>      }
>>      return;
>>  
>>  err:
>> +    pcie_aer_exit(d);
>>      pcie_chassis_del_slot(s);
>>  err_pcie_cap:
>>      pcie_cap_exit(d);
>> @@ -151,6 +159,7 @@ static void rp_exit(PCIDevice *d)
>>  static const Property rp_props[] = {
>>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> +    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
>>  };
>>  
>>  static void rp_instance_post_init(Object *obj)
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>> index 6a9df5c174..e243dfdf7d 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -58,11 +58,12 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
>>  static void xio3130_downstream_reset(DeviceState *qdev)
>>  {
>>      PCIDevice *d = PCI_DEVICE(qdev);
>> +    PCIEPort *p = PCIE_PORT(d);
>>  
>>      pcie_cap_deverr_reset(d);
>>      pcie_cap_slot_reset(d);
>>      pcie_cap_arifwd_reset(d);
>> -    pcie_acs_reset(d);
>> +    pcie_acs_reset(d, p->acs_ctrl);
>>      pci_bridge_reset(qdev);
>>  }
>>  
>> @@ -115,12 +116,19 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>>      }
>>  
>>      if (d->cap_present & QEMU_PCIE_CAP_ACS) {
>> -        pcie_acs_init(d, XIO3130_ACS_OFFSET);
>> +        rc = pcie_acs_init(d, XIO3130_ACS_OFFSET, p->acs_ctrl, errp);
>> +        if (rc < 0) {
>> +            goto err;
>> +        }
>> +    } else if (p->acs_ctrl) {
>> +        error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
>> +        goto err;
> 
> Tying a stable QAPI to an unstable one like this is not really
> reasonable.


My bad. It is not tied to x-pcie-acs. The error message is worded
incorrectly. The check (added as per Shameer's feedback on V1) is there
so that acs-ctrl is not silently ignored on a port without ACS, so the
message should simply say that ACS is not supported by this port. I will
fix the message.

Thanks.
-Tushar


> 
>>      }
>>  
>>      return;
>>  
>>  err:
>> +    pcie_aer_exit(d);
>>      pcie_chassis_del_slot(s);
>>  err_pcie_cap:
>>      pcie_cap_exit(d);
>> @@ -146,6 +154,7 @@ static const Property xio3130_downstream_props[] = {
>>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>>      DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
>>                      QEMU_PCIE_ACS_BITNR, true),
>> +    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
>>  };
>>  
>>  static const VMStateDescription vmstate_xio3130_downstream = {
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 42bcb9206d..af24ec7fd9 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -1260,7 +1260,8 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned)
>>  }
>>  
>>  /* ACS (Access Control Services) */
>> -void pcie_acs_init(PCIDevice *dev, uint16_t offset)
>> +int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>> +                  Error **errp)
>>  {
>>      bool is_downstream = pci_is_express_downstream_port(dev);
>>      uint16_t cap_bits = 0;
>> @@ -1284,16 +1285,26 @@ void pcie_acs_init(PCIDevice *dev, uint16_t offset)
>>           */
>>          cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
>>              PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
>> +
>> +        if (ctrl_bits & ~cap_bits) {
>> +            error_setg(errp,
>> +                       "acs-ctrl 0x%hx has bits not supported by this port "
>> +                       "(supported: 0x%hx)", ctrl_bits, cap_bits);
>> +            return -EINVAL;
>> +        }
>>      }
>>  
>>      pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
>>      pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
>> +    pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
>> +
>> +    return 0;
>>  }
>>  
>> -void pcie_acs_reset(PCIDevice *dev)
>> +void pcie_acs_reset(PCIDevice *dev, uint16_t val)
>>  {
>>      if (dev->exp.acs_cap) {
>> -        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
>> +        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, val);
>>      }
>>  }
>>  
>> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
>> index ec25e7a7de..678ad73f7f 100644
>> --- a/include/hw/pci/pcie.h
>> +++ b/include/hw/pci/pcie.h
>> @@ -137,8 +137,9 @@ bool pcie_insert_capability(PCIDevice *dev, uint16_t cap_id, uint8_t cap_ver,
>>                              uint16_t offset, uint16_t size);
>>  void pcie_sync_bridge_lnk(PCIDevice *dev);
>>  
>> -void pcie_acs_init(PCIDevice *dev, uint16_t offset);
>> -void pcie_acs_reset(PCIDevice *dev);
>> +int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>> +                  Error **errp);
>> +void pcie_acs_reset(PCIDevice *dev, uint16_t val);
>>  
>>  void pcie_ari_init(PCIDevice *dev, uint16_t offset);
>>  void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
>> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>> index b28af067a6..2ad9a563d9 100644
>> --- a/include/hw/pci/pcie_port.h
>> +++ b/include/hw/pci/pcie_port.h
>> @@ -36,6 +36,7 @@ struct PCIEPort {
>>  
>>      /* pci express switch port */
>>      uint8_t     port;
>> +    uint16_t    acs_ctrl;
>>  };
>>  
>>  void pcie_port_init_reg(PCIDevice *d);
>> -- 
>> 2.34.1
> 

[PATCH v2 3/3] hw/pci: Make ACS Control read-only to guests

[Tushar Dave]

GPUDirect RDMA using data-direct requires a specific ACS configuration
on PCIe Root Ports and Downstream Ports.

While ACS can be configured via QEMU's 'acs-ctrl' property, the guest
kernel may overwrite ACS during standard programming.

This change blocks all guest writes to the PCIe ACS Control register and
preserves QEMU-provided ACS settings across device resets on PCIe Root Ports
and Downstream Ports.

Signed-off-by: Tushar Dave <tdave@nvidia.com>
---
 hw/pci-bridge/pcie_root_port.c     |  5 ++--
 hw/pci-bridge/xio3130_downstream.c |  5 ++--
 hw/pci/pcie.c                      | 46 ++++++++++++++++++++++++++++--
 include/hw/pci/pcie.h              |  3 ++
 include/hw/pci/pcie_port.h         |  1 +
 5 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index e7cce8c0ee..6ac64c8b2b 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -123,7 +123,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
         if (rc < 0) {
             goto err;
         }
-    } else if (p->acs_ctrl) {
+    } else if (p->acs_ctrl_configured) {
         error_setg(errp, "acs-ctrl is not supported by this root port");
         goto err;
     }
@@ -159,7 +159,8 @@ static void rp_exit(PCIDevice *d)
 static const Property rp_props[] = {
     DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
-    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
+    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
+                                   qdev_prop_acs_ctrl, uint16_t),
 };
 
 static void rp_instance_post_init(Object *obj)
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index e243dfdf7d..66d25098d7 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -120,7 +120,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
         if (rc < 0) {
             goto err;
         }
-    } else if (p->acs_ctrl) {
+    } else if (p->acs_ctrl_configured) {
         error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
         goto err;
     }
@@ -154,7 +154,8 @@ static const Property xio3130_downstream_props[] = {
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
     DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
                     QEMU_PCIE_ACS_BITNR, true),
-    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
+    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
+                                   qdev_prop_acs_ctrl, uint16_t),
 };
 
 static const VMStateDescription vmstate_xio3130_downstream = {
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index af24ec7fd9..1fead3a053 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -1265,6 +1265,8 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
 {
     bool is_downstream = pci_is_express_downstream_port(dev);
     uint16_t cap_bits = 0;
+    PCIEPort *p = (PCIEPort *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_PORT);
+    bool configured = p && p->acs_ctrl_configured;
 
     /* For endpoints, only multifunction devs may have an ACS capability: */
     assert(is_downstream ||
@@ -1286,7 +1288,7 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
         cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
             PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
 
-        if (ctrl_bits & ~cap_bits) {
+        if (configured && (ctrl_bits & ~cap_bits)) {
             error_setg(errp,
                        "acs-ctrl 0x%hx has bits not supported by this port "
                        "(supported: 0x%hx)", ctrl_bits, cap_bits);
@@ -1295,7 +1297,17 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
     }
 
     pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
-    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
+
+    if (is_downstream && configured) {
+        /*
+         * Block guest writes to ACS Control entirely to preserve QEMU
+         * ACS settings
+         */
+        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, 0);
+    } else {
+        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
+    }
+
     pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
 
     return 0;
@@ -1308,6 +1320,36 @@ void pcie_acs_reset(PCIDevice *dev, uint16_t val)
     }
 }
 
+static void set_acs_ctrl(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    PCIEPort *p = PCIE_PORT(obj);
+    const Property *prop = opaque;
+    uint16_t *ptr = object_field_prop_ptr(obj, prop);
+
+    if (!visit_type_uint16(v, name, ptr, errp)) {
+        return;
+    }
+    p->acs_ctrl_configured = true;
+}
+
+static void get_acs_ctrl(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    const Property *prop = opaque;
+    uint16_t *ptr = object_field_prop_ptr(obj, prop);
+
+    visit_type_uint16(v, name, ptr, errp);
+}
+
+const PropertyInfo qdev_prop_acs_ctrl = {
+    .type = "uint16",
+    .description = "PCIe ACS ctrl configuration (if set, even to 0, "
+                   "makes ACS Control read-only to the guest)",
+    .get = get_acs_ctrl,
+    .set = set_acs_ctrl,
+};
+
 void pcie_pasid_common_init(PCIDevice *dev, uint16_t offset,
                             uint8_t pasid_width, bool exec_perm, bool priv_mod)
 {
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 678ad73f7f..bbb11fd402 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -26,6 +26,7 @@
 #include "hw/pci/pcie_aer.h"
 #include "hw/pci/pcie_sriov.h"
 #include "hw/core/hotplug.h"
+#include "hw/core/qdev-properties.h"
 
 typedef struct PCIEPort PCIEPort;
 typedef struct PCIESlot PCIESlot;
@@ -86,6 +87,8 @@ struct PCIExpressDevice {
 
 #define COMPAT_PROP_PCP "power_controller_present"
 
+extern const PropertyInfo qdev_prop_acs_ctrl;
+
 /* PCI express capability helper functions */
 int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
                   uint8_t port, Error **errp);
diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
index 2ad9a563d9..ef2c2ec2cd 100644
--- a/include/hw/pci/pcie_port.h
+++ b/include/hw/pci/pcie_port.h
@@ -37,6 +37,7 @@ struct PCIEPort {
     /* pci express switch port */
     uint8_t     port;
     uint16_t    acs_ctrl;
+    bool        acs_ctrl_configured;
 };
 
 void pcie_port_init_reg(PCIDevice *d);
-- 
2.34.1

Re: [PATCH v2 3/3] hw/pci: Make ACS Control read-only to guests

[Michael S. Tsirkin]

On Mon, Oct 05, 2026 at 08:31:33PM -0500, Tushar Dave wrote:
> GPUDirect RDMA using data-direct requires a specific ACS configuration
> on PCIe Root Ports and Downstream Ports.
> 
> While ACS can be configured via QEMU's 'acs-ctrl' property, the guest
> kernel may overwrite ACS during standard programming.
> 
> This change blocks all guest writes to the PCIe ACS Control register and
> preserves QEMU-provided ACS settings across device resets on PCIe Root Ports
> and Downstream Ports.

Why is that a sensible thing to do?
per pci express spec:

7.16.3. ACS Control Register (Offset 06h)

Bit Location
0
Register Description
ACS Source Validation Enable (V) – When Set, the component
validates the Bus Number from the Requester ID of Upstream
Requests against the secondary/subordinate Bus Numbers.
Attributes
RW
Default value of this bit is 0b. Must be hardwired to 0b if the ACS
Source Validation functionality is not implemented.


And this is repeated for every bit:

RW
Default value of this bit is 0b. Must be hardwired to 0b if the ACS
Source Validation functionality is not implemented.



Looks like your change would put us out of spec?


> Signed-off-by: Tushar Dave <tdave@nvidia.com>
> ---
>  hw/pci-bridge/pcie_root_port.c     |  5 ++--
>  hw/pci-bridge/xio3130_downstream.c |  5 ++--
>  hw/pci/pcie.c                      | 46 ++++++++++++++++++++++++++++--
>  include/hw/pci/pcie.h              |  3 ++
>  include/hw/pci/pcie_port.h         |  1 +
>  5 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index e7cce8c0ee..6ac64c8b2b 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -123,7 +123,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
>          if (rc < 0) {
>              goto err;
>          }
> -    } else if (p->acs_ctrl) {
> +    } else if (p->acs_ctrl_configured) {
>          error_setg(errp, "acs-ctrl is not supported by this root port");
>          goto err;
>      }
> @@ -159,7 +159,8 @@ static void rp_exit(PCIDevice *d)
>  static const Property rp_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> -    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
> +    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
> +                                   qdev_prop_acs_ctrl, uint16_t),
>  };
>  
>  static void rp_instance_post_init(Object *obj)
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index e243dfdf7d..66d25098d7 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -120,7 +120,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>          if (rc < 0) {
>              goto err;
>          }
> -    } else if (p->acs_ctrl) {
> +    } else if (p->acs_ctrl_configured) {
>          error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
>          goto err;
>      }
> @@ -154,7 +154,8 @@ static const Property xio3130_downstream_props[] = {
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>      DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
>                      QEMU_PCIE_ACS_BITNR, true),
> -    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
> +    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
> +                                   qdev_prop_acs_ctrl, uint16_t),
>  };
>  
>  static const VMStateDescription vmstate_xio3130_downstream = {
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index af24ec7fd9..1fead3a053 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -1265,6 +1265,8 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>  {
>      bool is_downstream = pci_is_express_downstream_port(dev);
>      uint16_t cap_bits = 0;
> +    PCIEPort *p = (PCIEPort *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_PORT);
> +    bool configured = p && p->acs_ctrl_configured;
>  
>      /* For endpoints, only multifunction devs may have an ACS capability: */
>      assert(is_downstream ||
> @@ -1286,7 +1288,7 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>          cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
>              PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
>  
> -        if (ctrl_bits & ~cap_bits) {
> +        if (configured && (ctrl_bits & ~cap_bits)) {
>              error_setg(errp,
>                         "acs-ctrl 0x%hx has bits not supported by this port "
>                         "(supported: 0x%hx)", ctrl_bits, cap_bits);
> @@ -1295,7 +1297,17 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>      }
>  
>      pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
> -    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> +
> +    if (is_downstream && configured) {
> +        /*
> +         * Block guest writes to ACS Control entirely to preserve QEMU
> +         * ACS settings
> +         */
> +        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, 0);
> +    } else {
> +        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> +    }
> +
>      pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
>  
>      return 0;
> @@ -1308,6 +1320,36 @@ void pcie_acs_reset(PCIDevice *dev, uint16_t val)
>      }
>  }
>  
> +static void set_acs_ctrl(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    PCIEPort *p = PCIE_PORT(obj);
> +    const Property *prop = opaque;
> +    uint16_t *ptr = object_field_prop_ptr(obj, prop);
> +
> +    if (!visit_type_uint16(v, name, ptr, errp)) {
> +        return;
> +    }
> +    p->acs_ctrl_configured = true;
> +}
> +
> +static void get_acs_ctrl(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    const Property *prop = opaque;
> +    uint16_t *ptr = object_field_prop_ptr(obj, prop);
> +
> +    visit_type_uint16(v, name, ptr, errp);
> +}
> +
> +const PropertyInfo qdev_prop_acs_ctrl = {
> +    .type = "uint16",
> +    .description = "PCIe ACS ctrl configuration (if set, even to 0, "
> +                   "makes ACS Control read-only to the guest)",
> +    .get = get_acs_ctrl,
> +    .set = set_acs_ctrl,
> +};
> +
>  void pcie_pasid_common_init(PCIDevice *dev, uint16_t offset,
>                              uint8_t pasid_width, bool exec_perm, bool priv_mod)
>  {
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 678ad73f7f..bbb11fd402 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -26,6 +26,7 @@
>  #include "hw/pci/pcie_aer.h"
>  #include "hw/pci/pcie_sriov.h"
>  #include "hw/core/hotplug.h"
> +#include "hw/core/qdev-properties.h"
>  
>  typedef struct PCIEPort PCIEPort;
>  typedef struct PCIESlot PCIESlot;
> @@ -86,6 +87,8 @@ struct PCIExpressDevice {
>  
>  #define COMPAT_PROP_PCP "power_controller_present"
>  
> +extern const PropertyInfo qdev_prop_acs_ctrl;
> +
>  /* PCI express capability helper functions */
>  int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
>                    uint8_t port, Error **errp);
> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> index 2ad9a563d9..ef2c2ec2cd 100644
> --- a/include/hw/pci/pcie_port.h
> +++ b/include/hw/pci/pcie_port.h
> @@ -37,6 +37,7 @@ struct PCIEPort {
>      /* pci express switch port */
>      uint8_t     port;
>      uint16_t    acs_ctrl;
> +    bool        acs_ctrl_configured;
>  };
>  
>  void pcie_port_init_reg(PCIDevice *d);
> -- 
> 2.34.1

Re: [PATCH v2 3/3] hw/pci: Make ACS Control read-only to guests

[Tushar Dave]

On 10/5/2026 11:44 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 05, 2026 at 08:31:33PM -0500, Tushar Dave wrote:
>> GPUDirect RDMA using data-direct requires a specific ACS configuration
>> on PCIe Root Ports and Downstream Ports.
>>
>> While ACS can be configured via QEMU's 'acs-ctrl' property, the guest
>> kernel may overwrite ACS during standard programming.
>>
>> This change blocks all guest writes to the PCIe ACS Control register and
>> preserves QEMU-provided ACS settings across device resets on PCIe Root Ports
>> and Downstream Ports.
> 
> Why is that a sensible thing to do?


It is sensible because it allows the administrator to define and
preserve the PCIe topology and P2P configuration from the QEMU command
line. The administrator can configure ACS on a Root Port or Downstream
Port as part of defining the desired PCIe topology and P2P behavior. If
the guest could modify the ACS Control register, it could change the
administrator-defined ACS configuration, which could prevent P2P from
working as expected.


> per pci express spec:
> 
> 7.16.3. ACS Control Register (Offset 06h)
> 
> Bit Location
> 0
> Register Description
> ACS Source Validation Enable (V) – When Set, the component
> validates the Bus Number from the Requester ID of Upstream
> Requests against the secondary/subordinate Bus Numbers.
> Attributes
> RW
> Default value of this bit is 0b. Must be hardwired to 0b if the ACS
> Source Validation functionality is not implemented.
> 
> 
> And this is repeated for every bit:
> 
> RW
> Default value of this bit is 0b. Must be hardwired to 0b if the ACS
> Source Validation functionality is not implemented.
> 
> 
> 
> Looks like your change would put us out of spec?


You are right that the ACS Control bits are defined as RW, and I am not
claiming otherwise. The intent is not to make ACS Control read-only in
general. The read-only behavior is tied to the acs-ctrl property:

- If acs-ctrl is not specified, ACS Control remains writable as it is
  today.
- If acs-ctrl is explicitly specified, the administrator has requested a
  specific ACS configuration. QEMU then preserves that configuration and
  does not allow the guest to modify it.

This is an intentional virtualization policy. When the administrator
explicitly configures the ACS Control register, the guest should not be
able to change the ACS configuration and P2P behavior selected by the
administrator.

Thanks.
-Tushar

> 
> 
>> Signed-off-by: Tushar Dave <tdave@nvidia.com>
>> ---
>>  hw/pci-bridge/pcie_root_port.c     |  5 ++--
>>  hw/pci-bridge/xio3130_downstream.c |  5 ++--
>>  hw/pci/pcie.c                      | 46 ++++++++++++++++++++++++++++--
>>  include/hw/pci/pcie.h              |  3 ++
>>  include/hw/pci/pcie_port.h         |  1 +
>>  5 files changed, 54 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> index e7cce8c0ee..6ac64c8b2b 100644
>> --- a/hw/pci-bridge/pcie_root_port.c
>> +++ b/hw/pci-bridge/pcie_root_port.c
>> @@ -123,7 +123,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
>>          if (rc < 0) {
>>              goto err;
>>          }
>> -    } else if (p->acs_ctrl) {
>> +    } else if (p->acs_ctrl_configured) {
>>          error_setg(errp, "acs-ctrl is not supported by this root port");
>>          goto err;
>>      }
>> @@ -159,7 +159,8 @@ static void rp_exit(PCIDevice *d)
>>  static const Property rp_props[] = {
>>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> -    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
>> +    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
>> +                                   qdev_prop_acs_ctrl, uint16_t),
>>  };
>>  
>>  static void rp_instance_post_init(Object *obj)
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>> index e243dfdf7d..66d25098d7 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -120,7 +120,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>>          if (rc < 0) {
>>              goto err;
>>          }
>> -    } else if (p->acs_ctrl) {
>> +    } else if (p->acs_ctrl_configured) {
>>          error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
>>          goto err;
>>      }
>> @@ -154,7 +154,8 @@ static const Property xio3130_downstream_props[] = {
>>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>>      DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
>>                      QEMU_PCIE_ACS_BITNR, true),
>> -    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
>> +    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
>> +                                   qdev_prop_acs_ctrl, uint16_t),
>>  };
>>  
>>  static const VMStateDescription vmstate_xio3130_downstream = {
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index af24ec7fd9..1fead3a053 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -1265,6 +1265,8 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>>  {
>>      bool is_downstream = pci_is_express_downstream_port(dev);
>>      uint16_t cap_bits = 0;
>> +    PCIEPort *p = (PCIEPort *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_PORT);
>> +    bool configured = p && p->acs_ctrl_configured;
>>  
>>      /* For endpoints, only multifunction devs may have an ACS capability: */
>>      assert(is_downstream ||
>> @@ -1286,7 +1288,7 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>>          cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
>>              PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
>>  
>> -        if (ctrl_bits & ~cap_bits) {
>> +        if (configured && (ctrl_bits & ~cap_bits)) {
>>              error_setg(errp,
>>                         "acs-ctrl 0x%hx has bits not supported by this port "
>>                         "(supported: 0x%hx)", ctrl_bits, cap_bits);
>> @@ -1295,7 +1297,17 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
>>      }
>>  
>>      pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
>> -    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
>> +
>> +    if (is_downstream && configured) {
>> +        /*
>> +         * Block guest writes to ACS Control entirely to preserve QEMU
>> +         * ACS settings
>> +         */
>> +        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, 0);
>> +    } else {
>> +        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
>> +    }
>> +
>>      pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
>>  
>>      return 0;
>> @@ -1308,6 +1320,36 @@ void pcie_acs_reset(PCIDevice *dev, uint16_t val)
>>      }
>>  }
>>  
>> +static void set_acs_ctrl(Object *obj, Visitor *v, const char *name,
>> +                         void *opaque, Error **errp)
>> +{
>> +    PCIEPort *p = PCIE_PORT(obj);
>> +    const Property *prop = opaque;
>> +    uint16_t *ptr = object_field_prop_ptr(obj, prop);
>> +
>> +    if (!visit_type_uint16(v, name, ptr, errp)) {
>> +        return;
>> +    }
>> +    p->acs_ctrl_configured = true;
>> +}
>> +
>> +static void get_acs_ctrl(Object *obj, Visitor *v, const char *name,
>> +                         void *opaque, Error **errp)
>> +{
>> +    const Property *prop = opaque;
>> +    uint16_t *ptr = object_field_prop_ptr(obj, prop);
>> +
>> +    visit_type_uint16(v, name, ptr, errp);
>> +}
>> +
>> +const PropertyInfo qdev_prop_acs_ctrl = {
>> +    .type = "uint16",
>> +    .description = "PCIe ACS ctrl configuration (if set, even to 0, "
>> +                   "makes ACS Control read-only to the guest)",
>> +    .get = get_acs_ctrl,
>> +    .set = set_acs_ctrl,
>> +};
>> +
>>  void pcie_pasid_common_init(PCIDevice *dev, uint16_t offset,
>>                              uint8_t pasid_width, bool exec_perm, bool priv_mod)
>>  {
>> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
>> index 678ad73f7f..bbb11fd402 100644
>> --- a/include/hw/pci/pcie.h
>> +++ b/include/hw/pci/pcie.h
>> @@ -26,6 +26,7 @@
>>  #include "hw/pci/pcie_aer.h"
>>  #include "hw/pci/pcie_sriov.h"
>>  #include "hw/core/hotplug.h"
>> +#include "hw/core/qdev-properties.h"
>>  
>>  typedef struct PCIEPort PCIEPort;
>>  typedef struct PCIESlot PCIESlot;
>> @@ -86,6 +87,8 @@ struct PCIExpressDevice {
>>  
>>  #define COMPAT_PROP_PCP "power_controller_present"
>>  
>> +extern const PropertyInfo qdev_prop_acs_ctrl;
>> +
>>  /* PCI express capability helper functions */
>>  int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
>>                    uint8_t port, Error **errp);
>> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>> index 2ad9a563d9..ef2c2ec2cd 100644
>> --- a/include/hw/pci/pcie_port.h
>> +++ b/include/hw/pci/pcie_port.h
>> @@ -37,6 +37,7 @@ struct PCIEPort {
>>      /* pci express switch port */
>>      uint8_t     port;
>>      uint16_t    acs_ctrl;
>> +    bool        acs_ctrl_configured;
>>  };
>>  
>>  void pcie_port_init_reg(PCIDevice *d);
>> -- 
>> 2.34.1
> 

Re: [PATCH v2 3/3] hw/pci: Make ACS Control read-only to guests

[Michael S. Tsirkin]

On Wed, Oct 07, 2026 at 11:33:04AM -0500, Tushar Dave wrote:
> 
> 
> On 10/5/2026 11:44 PM, Michael S. Tsirkin wrote:
> > On Mon, Oct 05, 2026 at 08:31:33PM -0500, Tushar Dave wrote:
> >> GPUDirect RDMA using data-direct requires a specific ACS configuration
> >> on PCIe Root Ports and Downstream Ports.
> >>
> >> While ACS can be configured via QEMU's 'acs-ctrl' property, the guest
> >> kernel may overwrite ACS during standard programming.
> >>
> >> This change blocks all guest writes to the PCIe ACS Control register and
> >> preserves QEMU-provided ACS settings across device resets on PCIe Root Ports
> >> and Downstream Ports.
> > 
> > Why is that a sensible thing to do?
> 
> 
> It is sensible because it allows the administrator to define and
> preserve the PCIe topology and P2P configuration from the QEMU command
> line. The administrator can configure ACS on a Root Port or Downstream
> Port as part of defining the desired PCIe topology and P2P behavior. If
> the guest could modify the ACS Control register, it could change the
> administrator-defined ACS configuration, which could prevent P2P from
> working as expected.
> 
> > per pci express spec:
> > 
> > 7.16.3. ACS Control Register (Offset 06h)
> > 
> > Bit Location
> > 0
> > Register Description
> > ACS Source Validation Enable (V) – When Set, the component
> > validates the Bus Number from the Requester ID of Upstream
> > Requests against the secondary/subordinate Bus Numbers.
> > Attributes
> > RW
> > Default value of this bit is 0b. Must be hardwired to 0b if the ACS
> > Source Validation functionality is not implemented.
> > 
> > 
> > And this is repeated for every bit:
> > 
> > RW
> > Default value of this bit is 0b. Must be hardwired to 0b if the ACS
> > Source Validation functionality is not implemented.
> > 
> > 
> > 
> > Looks like your change would put us out of spec?
> 
> 
> You are right that the ACS Control bits are defined as RW, and I am not
> claiming otherwise. The intent is not to make ACS Control read-only in
> general. The read-only behavior is tied to the acs-ctrl property:
> 
> - If acs-ctrl is not specified, ACS Control remains writable as it is
>   today.
> - If acs-ctrl is explicitly specified, the administrator has requested a
>   specific ACS configuration. QEMU then preserves that configuration and
>   does not allow the guest to modify it.
> 
> This is an intentional virtualization policy. When the administrator
> explicitly configures the ACS Control register, the guest should not be
> able to change the ACS configuration and P2P behavior selected by the
> administrator.
> 
> Thanks.
> -Tushar

And then some guest writes the register, reads the value expecting to
see what it wrote, and crashes.  I don't care that it happens to work
for you, we are not doing this kind of thing.

> > 
> > 
> >> Signed-off-by: Tushar Dave <tdave@nvidia.com>
> >> ---
> >>  hw/pci-bridge/pcie_root_port.c     |  5 ++--
> >>  hw/pci-bridge/xio3130_downstream.c |  5 ++--
> >>  hw/pci/pcie.c                      | 46 ++++++++++++++++++++++++++++--
> >>  include/hw/pci/pcie.h              |  3 ++
> >>  include/hw/pci/pcie_port.h         |  1 +
> >>  5 files changed, 54 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> >> index e7cce8c0ee..6ac64c8b2b 100644
> >> --- a/hw/pci-bridge/pcie_root_port.c
> >> +++ b/hw/pci-bridge/pcie_root_port.c
> >> @@ -123,7 +123,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
> >>          if (rc < 0) {
> >>              goto err;
> >>          }
> >> -    } else if (p->acs_ctrl) {
> >> +    } else if (p->acs_ctrl_configured) {
> >>          error_setg(errp, "acs-ctrl is not supported by this root port");
> >>          goto err;
> >>      }
> >> @@ -159,7 +159,8 @@ static void rp_exit(PCIDevice *d)
> >>  static const Property rp_props[] = {
> >>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> >>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> >> -    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
> >> +    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
> >> +                                   qdev_prop_acs_ctrl, uint16_t),
> >>  };
> >>  
> >>  static void rp_instance_post_init(Object *obj)
> >> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> >> index e243dfdf7d..66d25098d7 100644
> >> --- a/hw/pci-bridge/xio3130_downstream.c
> >> +++ b/hw/pci-bridge/xio3130_downstream.c
> >> @@ -120,7 +120,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> >>          if (rc < 0) {
> >>              goto err;
> >>          }
> >> -    } else if (p->acs_ctrl) {
> >> +    } else if (p->acs_ctrl_configured) {
> >>          error_setg(errp, "acs-ctrl requires x-pcie-acs to be on");
> >>          goto err;
> >>      }
> >> @@ -154,7 +154,8 @@ static const Property xio3130_downstream_props[] = {
> >>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> >>      DEFINE_PROP_BIT("x-pcie-acs", PCIDevice, cap_present,
> >>                      QEMU_PCIE_ACS_BITNR, true),
> >> -    DEFINE_PROP_UINT16("acs-ctrl", PCIEPort, acs_ctrl, 0),
> >> +    DEFINE_PROP_UNSIGNED_NODEFAULT("acs-ctrl", PCIEPort, acs_ctrl,
> >> +                                   qdev_prop_acs_ctrl, uint16_t),
> >>  };
> >>  
> >>  static const VMStateDescription vmstate_xio3130_downstream = {
> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >> index af24ec7fd9..1fead3a053 100644
> >> --- a/hw/pci/pcie.c
> >> +++ b/hw/pci/pcie.c
> >> @@ -1265,6 +1265,8 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
> >>  {
> >>      bool is_downstream = pci_is_express_downstream_port(dev);
> >>      uint16_t cap_bits = 0;
> >> +    PCIEPort *p = (PCIEPort *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_PORT);
> >> +    bool configured = p && p->acs_ctrl_configured;
> >>  
> >>      /* For endpoints, only multifunction devs may have an ACS capability: */
> >>      assert(is_downstream ||
> >> @@ -1286,7 +1288,7 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
> >>          cap_bits = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> >>              PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT;
> >>  
> >> -        if (ctrl_bits & ~cap_bits) {
> >> +        if (configured && (ctrl_bits & ~cap_bits)) {
> >>              error_setg(errp,
> >>                         "acs-ctrl 0x%hx has bits not supported by this port "
> >>                         "(supported: 0x%hx)", ctrl_bits, cap_bits);
> >> @@ -1295,7 +1297,17 @@ int pcie_acs_init(PCIDevice *dev, uint16_t offset, uint16_t ctrl_bits,
> >>      }
> >>  
> >>      pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
> >> -    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> >> +
> >> +    if (is_downstream && configured) {
> >> +        /*
> >> +         * Block guest writes to ACS Control entirely to preserve QEMU
> >> +         * ACS settings
> >> +         */
> >> +        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, 0);
> >> +    } else {
> >> +        pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> >> +    }
> >> +
> >>      pci_set_word(dev->config + offset + PCI_ACS_CTRL, ctrl_bits);
> >>  
> >>      return 0;
> >> @@ -1308,6 +1320,36 @@ void pcie_acs_reset(PCIDevice *dev, uint16_t val)
> >>      }
> >>  }
> >>  
> >> +static void set_acs_ctrl(Object *obj, Visitor *v, const char *name,
> >> +                         void *opaque, Error **errp)
> >> +{
> >> +    PCIEPort *p = PCIE_PORT(obj);
> >> +    const Property *prop = opaque;
> >> +    uint16_t *ptr = object_field_prop_ptr(obj, prop);
> >> +
> >> +    if (!visit_type_uint16(v, name, ptr, errp)) {
> >> +        return;
> >> +    }
> >> +    p->acs_ctrl_configured = true;
> >> +}
> >> +
> >> +static void get_acs_ctrl(Object *obj, Visitor *v, const char *name,
> >> +                         void *opaque, Error **errp)
> >> +{
> >> +    const Property *prop = opaque;
> >> +    uint16_t *ptr = object_field_prop_ptr(obj, prop);
> >> +
> >> +    visit_type_uint16(v, name, ptr, errp);
> >> +}
> >> +
> >> +const PropertyInfo qdev_prop_acs_ctrl = {
> >> +    .type = "uint16",
> >> +    .description = "PCIe ACS ctrl configuration (if set, even to 0, "
> >> +                   "makes ACS Control read-only to the guest)",
> >> +    .get = get_acs_ctrl,
> >> +    .set = set_acs_ctrl,
> >> +};
> >> +
> >>  void pcie_pasid_common_init(PCIDevice *dev, uint16_t offset,
> >>                              uint8_t pasid_width, bool exec_perm, bool priv_mod)
> >>  {
> >> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> >> index 678ad73f7f..bbb11fd402 100644
> >> --- a/include/hw/pci/pcie.h
> >> +++ b/include/hw/pci/pcie.h
> >> @@ -26,6 +26,7 @@
> >>  #include "hw/pci/pcie_aer.h"
> >>  #include "hw/pci/pcie_sriov.h"
> >>  #include "hw/core/hotplug.h"
> >> +#include "hw/core/qdev-properties.h"
> >>  
> >>  typedef struct PCIEPort PCIEPort;
> >>  typedef struct PCIESlot PCIESlot;
> >> @@ -86,6 +87,8 @@ struct PCIExpressDevice {
> >>  
> >>  #define COMPAT_PROP_PCP "power_controller_present"
> >>  
> >> +extern const PropertyInfo qdev_prop_acs_ctrl;
> >> +
> >>  /* PCI express capability helper functions */
> >>  int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
> >>                    uint8_t port, Error **errp);
> >> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> >> index 2ad9a563d9..ef2c2ec2cd 100644
> >> --- a/include/hw/pci/pcie_port.h
> >> +++ b/include/hw/pci/pcie_port.h
> >> @@ -37,6 +37,7 @@ struct PCIEPort {
> >>      /* pci express switch port */
> >>      uint8_t     port;
> >>      uint16_t    acs_ctrl;
> >> +    bool        acs_ctrl_configured;
> >>  };
> >>  
> >>  void pcie_port_init_reg(PCIDevice *d);
> >> -- 
> >> 2.34.1
> >