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 <[email protected]>
>>
>> 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 <[email protected]>
>> Co-developed-by: Tushar Dave <[email protected]>
>> Signed-off-by: Tushar Dave <[email protected]>
>> ---
>> 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
>
