diff mbox series

[v9,5/6] PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

Message ID SI2PR01MB43939FB8FF7F1239925023C4DCB62@SI2PR01MB4393.apcprd01.prod.exchangelabs.com
State New
Headers show
Series PCI: Add support for ACS Enhanced Capability | expand

Commit Message

Wei Wang Sept. 3, 2026, 3:46 a.m. UTC 
The ACS Enhanced Capability introduces several new access controls to
improve device isolation. These new controls are particularly important
for device passthrough in virtualization scenarios.

For example, a DMA transaction from a device may target a guest physical
address that lies within the memory aperture of the switch's upstream
port, but not within any memory aperture or BAR space of a downstream
port. In such cases, the switch would generate an Unsupported Request (UR)
response to the device, which is undesirable. Enabling Unclaimed Request
Redirect Control ensures that these DMA requests are forwarded upstream
instead of being rejected.

The ACS DSP and USP Memory Target Access Control and ACS I/O Request
Blocking features similarly enhance device isolation. Device grouping in
Linux assumes that devices are properly isolated. Therefore, enable these
controls by default if PCI_ACS_ECAP is supported by the hardware. As with
other basic ACS access controls, these new controls can be configured via
the "config_acs=" boot parameter.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 .../admin-guide/kernel-parameters.txt         | 23 +++++++++----
 drivers/pci/pci.c                             | 34 +++++++++++++++++--
 include/uapi/linux/pci_regs.h                 | 13 +++++++
 3 files changed, 61 insertions(+), 9 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 3, 2026, 4:39 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional validation of DMAC and UMAC fields rejects valid user ACS configurations on non-ECAP devices and fails to account for kernel-corrected states.
- [Medium] ACS Enhanced Capability access controls are unconditionally applied to unsupported PCIe port types, writing to reserved bits.
--

commit e1302a2bae5cac8f4c54de9a2a416f24d9b8a517
Author: Wei Wang <wei.w.wang@hotmail.com>

PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

The ACS Enhanced Capability introduces several new access controls to
improve device isolation. These new controls are particularly important
for device passthrough in virtualization scenarios.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6cffdbba99d34..50a3b61f9898e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1026,10 +1029,23 @@ static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
>  		disabled_bits &= valid_ctrl;
>  	}
>  
> +	/*
> +	 * DMAC/UMAC are 2-bit fields where encoding 0b11 is reserved. The
> +	 * final value can land on 0b11 not only when the user enables both
> +	 * bits, but also when they enable one bit, e.g. "1x", while fw_ctrl
> +	 * already has the other set (e.g. "01").
> +	 */
> +	new_ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
> +	if (FIELD_GET(PCI_ACS_DMAC_MASK, new_ctrl) == PCI_ACS_MAC_RSVD ||
> +	    FIELD_GET(PCI_ACS_UMAC_MASK, new_ctrl) == PCI_ACS_MAC_RSVD) {
> +		pci_err(dev, "ACS DMAC/UMAC would be set to reserved value\n");
> +		return;
> +	}

[Severity: High]
Does this unconditional check against reserved DMAC/UMAC values reject valid
user configurations on non-ECAP devices?

If a user boots with the config_acs parameter to configure a non-ECAP device,
but its firmware leaves bits 8:9 or 10:11 as 0b11, this FIELD_GET check detects
PCI_ACS_MAC_RSVD and aborts the configuration, preventing the user from
applying basic ACS settings.

Additionally, for ECAP devices with buggy firmware defaults (0b11), building
the new control register using caps->fw_ctrl instead of the kernel-corrected
caps->ctrl revives the invalid firmware state, instantly triggering this
validation failure and rejecting unrelated user flags.

[ ... ]

> @@ -1051,6 +1067,20 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>  	/* Upstream Forwarding */
>  	caps->ctrl |= (dev->acs_capabilities & PCI_ACS_UF);
>  
> +	/*
> +	 * Enable Unclaimed Request Redirect Control, I/O Request Blocking,
> +	 * and Downstream and Upstream Port Memory Target Access Redirect.
> +	 */
> +	if (dev->acs_capabilities & PCI_ACS_ECAP) {
> +		caps->ctrl |= PCI_ACS_IB;
> +		FIELD_MODIFY(PCI_ACS_DMAC_MASK, &caps->ctrl, PCI_ACS_MAC_RR);

[Severity: Medium]
Can this logic unconditionally apply these access controls to unsupported
port types?

If the kernel enumerates a PCIe Endpoint or Upstream Port that has the
PCI_ACS_ECAP bit set in its ACS capabilities, this unconditionally modifies
PCI_ACS_DMAC_MASK and sets PCI_ACS_IB.

Because ECAP controls like DMAC only apply to Root Ports and Downstream Ports,
applying them to Endpoints or Upstream Ports writes to reserved bits, which
could risk hardware misbehavior.

> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +			caps->ctrl |= PCI_ACS_URRC;
> +			FIELD_MODIFY(PCI_ACS_UMAC_MASK,
> +				     &caps->ctrl, PCI_ACS_MAC_RR);
> +		}
> +	}
> +
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a2563ff69f51..9019dac26b17 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5324,13 +5324,22 @@  Kernel parameters
 				flags.
 
 				ACS Flags is defined as follows:
-				  bit-0 : ACS Source Validation
-				  bit-1 : ACS Translation Blocking
-				  bit-2 : ACS P2P Request Redirect
-				  bit-3 : ACS P2P Completion Redirect
-				  bit-4 : ACS Upstream Forwarding
-				  bit-5 : ACS P2P Egress Control
-				  bit-6 : ACS Direct Translated P2P
+				  bit-0     : ACS Source Validation
+				  bit-1     : ACS Translation Blocking
+				  bit-2     : ACS P2P Request Redirect
+				  bit-3     : ACS P2P Completion Redirect
+				  bit-4     : ACS Upstream Forwarding
+				  bit-5     : ACS P2P Egress Control
+				  bit-6     : ACS Direct Translated P2P
+				  bit-7     : ACS I/O Request Blocking
+				  bit-9:8   : ACS DSP Memory Target Access Ctrl
+				      00    : Direct Request access enabled
+				      01    : Request blocking enabled
+				      10    : Request redirect enabled
+				      11    : Reserved
+				  bit-11:10 : ACS USP Memory Target Access Ctrl
+				              Same encoding as bit-9:8
+				  bit-12    : ACS Unclaimed Request Redirect Ctrl
 				Each bit can be marked as:
 				  '0' – force disabled
 				  '1' – force enabled
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6cffdbba99d3..50a3b61f9898 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -983,9 +983,12 @@  static const char *pci_dev_match_config_acs(struct pci_dev *dev, const char *p)
 static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
 {
 	u16 shift = 0, valid_ctrl = dev->acs_capabilities & GENMASK_U16(6, 0);
-	u16 invalid_bits, enabled_bits = 0, disabled_bits = 0;
+	u16 new_ctrl, invalid_bits, enabled_bits = 0, disabled_bits = 0;
 	const char *p, *seg;
 
+	if (dev->acs_capabilities & PCI_ACS_ECAP)
+		valid_ctrl |= GENMASK_U16(12, 7);
+
 	if (!config_acs_param || !valid_ctrl)
 		return;
 
@@ -1026,10 +1029,23 @@  static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
 		disabled_bits &= valid_ctrl;
 	}
 
+	/*
+	 * DMAC/UMAC are 2-bit fields where encoding 0b11 is reserved. The
+	 * final value can land on 0b11 not only when the user enables both
+	 * bits, but also when they enable one bit, e.g. "1x", while fw_ctrl
+	 * already has the other set (e.g. "01").
+	 */
+	new_ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
+	if (FIELD_GET(PCI_ACS_DMAC_MASK, new_ctrl) == PCI_ACS_MAC_RSVD ||
+	    FIELD_GET(PCI_ACS_UMAC_MASK, new_ctrl) == PCI_ACS_MAC_RSVD) {
+		pci_err(dev, "ACS DMAC/UMAC would be set to reserved value\n");
+		return;
+	}
+
 	pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n",
 		enabled_bits, disabled_bits);
 
-	caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
+	caps->ctrl = new_ctrl;
 }
 
 /**
@@ -1051,6 +1067,20 @@  static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
 	/* Upstream Forwarding */
 	caps->ctrl |= (dev->acs_capabilities & PCI_ACS_UF);
 
+	/*
+	 * Enable Unclaimed Request Redirect Control, I/O Request Blocking,
+	 * and Downstream and Upstream Port Memory Target Access Redirect.
+	 */
+	if (dev->acs_capabilities & PCI_ACS_ECAP) {
+		caps->ctrl |= PCI_ACS_IB;
+		FIELD_MODIFY(PCI_ACS_DMAC_MASK, &caps->ctrl, PCI_ACS_MAC_RR);
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
+			caps->ctrl |= PCI_ACS_URRC;
+			FIELD_MODIFY(PCI_ACS_UMAC_MASK,
+				     &caps->ctrl, PCI_ACS_MAC_RR);
+		}
+	}
+
 	/* Enable Translation Blocking for external devices and noats */
 	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
 		caps->ctrl |= (dev->acs_capabilities & PCI_ACS_TB);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index facaa324bd86..aeab8e5d54da 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1016,6 +1016,7 @@ 
 
 /* Access Control Service */
 #define PCI_ACS_CAP		0x04	/* ACS Capability Register */
+#define  PCI_ACS_ECAP		0x0080  /* ACS Enhanced Capability (CAP reg) */
 #define  PCI_ACS_SV		0x0001	/* Source Validation */
 #define  PCI_ACS_TB		0x0002	/* Translation Blocking */
 #define  PCI_ACS_RR		0x0004	/* P2P Request Redirect */
@@ -1023,10 +1024,22 @@ 
 #define  PCI_ACS_UF		0x0010	/* Upstream Forwarding */
 #define  PCI_ACS_EC		0x0020	/* P2P Egress Control */
 #define  PCI_ACS_DT		0x0040	/* Direct Translated P2P */
+#define  PCI_ACS_IB		0x0080	/* I/O Request Blocking (CTRL reg) */
+#define  PCI_ACS_DMAC_MASK	0x0300  /* DSP Memory Target Access Control */
+#define  PCI_ACS_UMAC_MASK	0x0C00  /* USP Memory Target Access Control */
+#define  PCI_ACS_URRC		0x1000	/* Unclaimed Request Redirect Ctrl */
 #define PCI_ACS_EGRESS_BITS	0x05	/* ACS Egress Control Vector Size */
 #define PCI_ACS_CTRL		0x06	/* ACS Control Register */
 #define PCI_ACS_EGRESS_CTL_V	0x08	/* ACS Egress Control Vector */
 
+/* Encodings for DSP and USP Memory Target Access Control */
+enum {
+	PCI_ACS_MAC_DA   = 0x0,		/* Direct request access */
+	PCI_ACS_MAC_RB   = 0x1,		/* Request blocking */
+	PCI_ACS_MAC_RR   = 0x2,		/* Request redirect */
+	PCI_ACS_MAC_RSVD = 0x3,		/* Reserved */
+};
+
 /* SATA capability */
 #define PCI_SATA_REGS		4	/* SATA REGs specifier */
 #define  PCI_SATA_REGS_MASK	0xF	/* location - BAR#/inline */