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

["Michael S. Tsirkin" <mst@redhat.com>]

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