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

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

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