[PATCH v10 0/6] PCI: Add support for ACS Enhanced Capability

[PATCH v10 0/6] PCI: Add support for ACS Enhanced Capability

[Wei Wang]

This patchset improves the core ACS implementation and adds support for
the Access Control Services (ACS) Enhanced Capability, introduced with
PCIe Gen 5.

Improvements to the core ACS implementation include:
- Validating ACS enable flags against device-specific capabilities rather
  than generic kernel masks. This ensures only supported features are
  enabled while safely ignoring attempts to disable unsupported bits.

- Consolidating delimiter parsing into pci_dev_str_match() so duplicated
  logic in callers can be removed.

- Refactoring ACS parameter handling by splitting the intertwined
  disable_acs_redir and config_acs param logic into dedicated functions.
  This improves maintainability and robustness while optimizing parsing
  with better validation and readability.

- Updating the config_acs kernel parameter documentation to include an
  example of multi-device configuration with distinct settings and
  advising users to quote the parameter to avoid bootloader parsing
  issues with the semicolon separator.

Support for the ACS Enhanced Capability is built on top of this improved
implementation. This capability provides additional access control
features that improve device isolation — particularly important in
virtualization scenarios where devices are passed through to different
virtual machines (VMs). Strong isolation is critical to ensure security
between devices assigned to different VMs and the host.

In Linux, device grouping assumes that devices in separate IOMMU groups
are properly isolated. To uphold this assumption, the enhanced ACS
controls are enabled by default on hardware that supports the PCI_ACS_ECAP
capability. As with other basic ACS access controls, these new controls
can be configured via the config_acs= boot parameter.

Support for checking the enhanced ACS controls on Root and Downstream
Ports has been added to pci_acs_enabled(). On devices that support
PCI_ACS_ECAP, these controls must be properly enabled. To maintain
compatibility with legacy devices that lack PCI_ACS_ECAP support,
pci_acs_enabled() simply skips the check.

v9->v10 changes:
Patch 1:
  - Replace GENMASK_U16(6, 0) with PCI_ACS_BASIC_CTRL_MASK, built from the
    individual PCI_ACS_* control bits, for readability. (Ilpo Järvinen)
  - Report ignored unsupported ACS bits with pci_warn() instead of
    pci_dbg(), so users can see that part of their config_acs= request
    had no effect (previously, bits above 6 were reported with
    pci_err()).
Patch 3:
  - Simplify pci_dev_str_match() by computing the vendor/device match
    result directly and dropping the goto labels; no change in
    behavior.
  - Reword the commit message: trailing characters after a BDF were
    already rejected by pci_dev_str_match_path(), so the stricter check
    only affects the pci:<vendor>:<device> form.
Patch 4:
  - Drop the unneeded "ret = 0" initializer and the else-after-return
    in pci_dev_match_disable_acs_redir().
Patch 5:
  - Replace GENMASK_U16(12, 7) with PCI_ACS_ECAP_CTRL_MASK for
    readability. (Ilpo Järvinen)
  v9 link: https://lore.kernel.org/all/SI2PR01MB4393F337A792CB3258C2E073DCB62@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/
v8->v9 changes:
Patch 1:
  - strip unsupported bits from both mask and flags rather than aborting
    the entire configuration. This aligns with the existing command-line
    configurations to have the supported bits applied at least (Sashiko)
Patch 4:
  - Preserve pre-refactor behavior for the empty-bitstring form, e.g.
   "@0000:01:00.0" in config_acs, which resets caps->ctrl to caps->fw_ctrl
    for the matched device (undoing pci_std_enable_acs()). Also restore the
   "ACS Flags missing" pci_err() diagnostic on a missing '@' delimiter.
   (Sashiko)
  - Two other comments from Sashiko were marked as non-critical and
    pre-existing; left unchanged for now to preserve the current behavior.
Patch 5:
  - Enable Unclaimed Request Redirect Control and Upstream Port Memory
    Target Access Redirect for Downstream Port only (PCI_ACS_ECAP is set
    in pci_acs_enabled() for root port and downstream port).(Sashiko)
  v8 Link: https://lore.kernel.org/all/SI2PR01MB43932642FC4DD3D075983B1FDC0A2@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

v7->v8 changes:
Patch 1:
  - Validate against dev->acs_capabilities & GENMASK_U16(6, 0) rather than
    the raw dev->acs_capabilities, as the latter may contain fields (like
    the egress control vector size) that do not correspond to bits in the
    ACS control register. (Sashiko-bot)
  - Gate the new capability check on !acs_mask so it only fires for the
    config_acs parameter path. This avoids breaking legacy
    disable_acs_redir behavior. (Sashiko-bot)
  - Validate both enable and disable bits (the full mask), not just flags.
    Reverts the v7 narrowing to enable-only.
Patch 2:
  - Remove the leading '-' characters in the documentation examples to
    prevent copy-paste confusion for users. (Randy)
  - Recommend using double quotes instead of single quotes for multiple
    devices to avoid potential shell-escaping issues where single quotes
    might be treated as literal characters. (Sashiko-bot)
Patch 3:
  - Move the error reporting back to the callers so that caller context
    is included in the dmesg output. (Krzysztof)
  - In pci_dev_str_match(): enforce stricter boundary checks on successful
    matches (see Patch 3 commit log for full details).
Patch 4:
  - Move the pci_info logging for ACS modifications up to pci_enable_acs()
    so it prints for both disable_acs_redir and config_acs modifications.
    Introduction of kernel_default_ctrl so the print only fires when a
    cmdline param diverges from pci_std_enable_acs()'s output (not on
    every pci_std_enable_acs() modification). (Sashiko-bot)
  - Drop max_shift; bound the parse loop by the u16 width (16) and rely on
    the existing invalid_bits & ~valid_ctrl check to report out-of-range
    bits with their bitmask, which is more informative than reporting the
    parse position.
  - Drop the -ENODEV "no more entries" signal. *endptr is now advanced
    past the delimiter so callers terminate the loop naturally via
    while (*p).
Patch 5:
  - In pci_param_config_acs(): when ECAP is supported by the device add
    the ECAP control bits (GENMASK_U16(12, 7)) to valid_ctrl.
  - Validate DMAC/UMAC reserved encoding against the prospective ctrl
    value rather than enabled_bits alone, so the case where the user
    enables one bit while fw_ctrl already has the other set is also
    caught.
Patch 6:
  - In pci_acs_ecap_enabled(): Add a defensive NULL check for the upstream
    port before dereferencing it. While standard downstream ports should
    always have an upstream port, this guards against kernel panics in
    malformed topologies. (Sashiko-bot)
  - Remove the redundant pdev->acs_capabilities & PCI_ACS_ECAP condition
    in pci_acs_flags_enabled(). The preceding bitwise operation
    (acs_flags &= (pdev->acs_capabilities | PCI_ACS_EC))
    already guarantees that acs_flags will only contain the PCI_ACS_ECAP
    bit if the device actually supports it.
  v7 Link: https://lore.kernel.org/all/SI2PR01MB439385689A32A1DDA9CEABE1DC3F2@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

v6->v7 changes:
 - Rebased onto next-20260505 (based on v7.1-rc2)
 - Picked up Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
 - No functional changes
 v6 Link: https://lore.kernel.org/all/SEZPR01MB439931D7320F6C476D181C1EDC44A@SEZPR01MB4399.apcprd01.prod.exchangelabs.com/

v5->v6 changes:
- Patch 3: In pci_dev_str_match(), explicitly set `ret = 0` when no
  matching string is found. This resolves a smatch warning that `ret`
  returned from sscanf() may be 2 or 4 even though no matching string
  is found later comparison.
  v5 Link: https://lore.kernel.org/all/SI2PR01MB439326AF08A79D1C5661C29BDC6CA@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

v4->v5 changes:
- Added significant refactoring of the core ACS implementation (Patches
  1-4) to improve validation, safety, and readability;
- For USP and DSP Memory Target Access Control, added masks and enum
  values for the encodings and explicitly rejected the reserved encoding
  (0b11);
- In pci_acs_ecap_enabled(), removed the use of 'is_dsp' variable.
  v4 Link: https://lore.kernel.org/all/SI2PR01MB43932C799AE9111C7D2C319FDC65A@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

v3->v4 changes:
- In pci_acs_ecap_enabled(): Check the pcie type for
  PCI_EXP_TYPE_DOWNSTREAM explicitly.
  v3 Link: https://lore.kernel.org/all/SI2PR01MB439325B4E44D5A39F34A4015DC9AA@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

v2->v3 changes:
- Drop the warning when a device has no support for the enhanced
  capability.
  v2 Link: https://lore.kernel.org/all/SI2PR01MB4393B836EA4FEDD1823483BADC94A@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

v1->v2 changes:
- Enabled all enhanced ACS controls by default, rather than just Unclaimed
  Request Redirect (which addressed the primary issue we encountered);
- Added checks for enhanced ACS controls on Root and Downstream Ports in
  pci_acs_enabled() to ensure proper enablement when grouping devices or
  enabling features such as IOMMU PASID.
  v1 Link: https://lore.kernel.org/all/SI2PR01MB43931A911357962A5E986FFEDC8CA@SI2PR01MB4393.apcprd01.prod.exchangelabs.com/

Thanks to Jason Gunthorpe, Jonathan Cameron, Dan Carpenter,
Krzysztof Wilczyński, Randy Dunlap and Ilpo Järvinen for reviewing
the patchset.

Patches on github: https://github.com/wei-w-wang/linux/tree/v10-acs-enhanced-cap-and-refactor

Wei Wang (6):
  PCI: Validate ACS control bits against device-specific ACS
    capabilities
  Documentation/kernel-parameters: Add multi-device config_acs example
  PCI: Consolidate delimiter handling into pci_dev_str_match()
  PCI: Refactor disable_acs_redir and config_acs param handling
  PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP
  PCI: Add the enhanced ACS controls check to pci_acs_flags_enabled()

 .../admin-guide/kernel-parameters.txt         |  30 +-
 drivers/pci/pci.c                             | 327 ++++++++++++------
 include/uapi/linux/pci_regs.h                 |  13 +
 3 files changed, 266 insertions(+), 104 deletions(-)


base-commit: a8c591ed6b672915e0be57843f943a2a723aff40
-- 
2.51.0

[PATCH v10 1/6] PCI: Validate ACS control bits against device-specific ACS capabilities

[Wei Wang]

The ACS control validation used the kernel's full set of ACS control bits,
which allowed users to request ACS features that the device does not
support. Because hardware silently ignores these unsupported bits, users
have no indication that their config_acs= request was partially
ineffective.

Validate the requested ACS control bits against dev->acs_capabilities so
that only device-supported ACS controls are accepted. Mask the capability
with PCI_ACS_BASIC_CTRL_MASK to select only the currently defined ACS
control bits (0-6). Higher bits in the capability structure (e.g. the
egress control vector size in bits 8-15) do not correspond to control bits
in the ACS control register. Warn about any unsupported bits that are
ignored.

__pci_config_acs() is also called from disable_acs_redir_param(), which
passes a hardcoded acs_mask (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC) that
does not need validation. Gate the new check on !acs_mask so it only
fires for the config_acs_param path, which always invokes
__pci_config_acs() with acs_mask == 0 and builds the actual mask from
user input.

Also move the check after the device is matched, since the ctrl bits apply
only to the matched device.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/pci/pci.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b2879a6be5f8..15a074e95504 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -37,6 +37,10 @@
 #include <linux/suspend.h>
 #include "pci.h"
 
+#define PCI_ACS_BASIC_CTRL_MASK \
+	(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | \
+	 PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)
+
 DEFINE_MUTEX(pci_slot_mutex);
 
 const char *pci_power_names[] = {
@@ -910,6 +914,7 @@ struct pci_acs {
 static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 			     const char *p, const u16 acs_mask, const u16 acs_flags)
 {
+	u16 valid_ctrl = dev->acs_capabilities & PCI_ACS_BASIC_CTRL_MASK;
 	u16 flags = acs_flags;
 	u16 mask = acs_mask;
 	char *delimit;
@@ -955,12 +960,6 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 			}
 		}
 
-		if (mask & ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
-			    PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)) {
-			pci_err(dev, "Invalid ACS flags specified\n");
-			return;
-		}
-
 		ret = pci_dev_str_match(dev, p, &p);
 		if (ret < 0) {
 			pr_warn_once("PCI: Can't parse ACS command line parameter\n");
@@ -983,6 +982,13 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 	if (!pci_dev_specific_disable_acs_redir(dev))
 		return;
 
+	if (!acs_mask && (mask & ~valid_ctrl)) {
+		pci_warn(dev, "Ignoring unsupported ACS bits: %#06x\n",
+			 mask & ~valid_ctrl);
+		mask &= valid_ctrl;
+		flags &= valid_ctrl;
+	}
+
 	pci_dbg(dev, "ACS mask  = %#06x\n", mask);
 	pci_dbg(dev, "ACS flags = %#06x\n", flags);
 	pci_dbg(dev, "ACS control = %#06x\n", caps->ctrl);
-- 
2.51.0

[PATCH v10 2/6] Documentation/kernel-parameters: Add multi-device config_acs example

[Wei Wang]

The config_acs parameter allows configuring ACS settings for multiple PCI
devices by separating entries with semicolons. The current documentation
only illustrates applying one configuration pattern to all devices, but
does not show how to specify multiple devices with different ACS settings
in a single parameter. Add an example demonstrating multi-device usage
with distinct ACS configurations.

Some bootloaders interpret ';' as a command separator, which can cause
the parameter to be split as multiple commands. Document that the entire
parameter may need to be quoted to avoid bootloader parsing issues. This
avoids confusing bootloader errors such as "can't find command <BDF>"
when passing multiple device entries.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2041dd9a8f18..8c4d3aa6fc67 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5352,6 +5352,13 @@ Kernel parameters
 
 				Note: this may remove isolation between devices
 				and may put more devices in an IOMMU group.
+
+				  "pci=config_acs=10x@0000:04:00.0;1x101@0000:84:00.0"
+				configures two devices with different ACS settings.
+				Note: Some bootloaders interpret ';' as a command
+				separator. If so, quote the entire parameter to
+				ensure it is passed to the kernel unchanged.
+
 		force_floating	[S390] Force usage of floating interrupts.
 		nomio		[S390] Do not use MIO instructions.
 		norid		[S390] ignore the RID field and force use of
-- 
2.51.0

[PATCH v10 3/6] PCI: Consolidate delimiter handling into pci_dev_str_match()

[Wei Wang]

Callers of pci_dev_str_match() manually checked for the ';' or ','
delimiter and advanced the parameter pointer past it. Move this common
logic into pci_dev_str_match() so callers no longer need to duplicate it.

As a side effect, pci_dev_str_match() now checks that a
pci:<vendor>:<device>[:<subvendor>:<subdevice>] token is immediately
followed by ';', ',' or the end of the string, and returns -EINVAL
otherwise. Previously, trailing characters after such a token (e.g.,
"pci=config_acs=110x@pci:10de:1234zz") were silently ignored: a
matching token was accepted as a match, and a non-matching one quietly
ended parsing of the rest of the parameter. Callers now report a parse
error in both cases.

The [<domain>:]<bus>:<device>.<func> form is unaffected, since
pci_dev_str_match_path() already rejects trailing characters.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/pci/pci.c | 43 ++++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 15a074e95504..3e2e34b24019 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -340,7 +340,7 @@ static int pci_dev_str_match_path(struct pci_dev *dev, const char *path,
  * pci_dev_str_match - test if a string matches a device
  * @dev: the PCI device to test
  * @p: string to match the device against
- * @endptr: pointer to the string after the match
+ * @endptr: pointer to the string after the match, with the delimiter skipped
  *
  * Test if a string (typically from a kernel parameter) matches a specified
  * PCI device. The string may be of one of the following formats:
@@ -389,13 +389,13 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
 
 		p += count;
 
-		if ((!vendor || vendor == dev->vendor) &&
-		    (!device || device == dev->device) &&
-		    (!subsystem_vendor ||
-			    subsystem_vendor == dev->subsystem_vendor) &&
-		    (!subsystem_device ||
-			    subsystem_device == dev->subsystem_device))
-			goto found;
+		/* ret = 0 if no matching string found */
+		ret = (!vendor || vendor == dev->vendor) &&
+		      (!device || device == dev->device) &&
+		      (!subsystem_vendor ||
+		       subsystem_vendor == dev->subsystem_vendor) &&
+		      (!subsystem_device ||
+		       subsystem_device == dev->subsystem_device);
 	} else {
 		/*
 		 * PCI Bus, Device, Function IDs are specified
@@ -404,16 +404,17 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
 		ret = pci_dev_str_match_path(dev, p, &p);
 		if (ret < 0)
 			return ret;
-		else if (ret)
-			goto found;
 	}
 
-	*endptr = p;
-	return 0;
+	/*
+	 * Whether we matched (ret == 1) or didn't (ret == 0),
+	 * ensure the token ends with a valid boundary.
+	 */
+	if (*p != '\0' && *p != ';' && *p != ',')
+		return -EINVAL;
 
-found:
-	*endptr = p;
-	return 1;
+	*endptr = *p == '\0' ? p : p + 1;
+	return ret;
 }
 
 static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
@@ -968,12 +969,6 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 			/* Found a match */
 			break;
 		}
-
-		if (*p != ';' && *p != ',') {
-			/* End of param or invalid format */
-			break;
-		}
-		p++;
 	}
 
 	if (ret != 1)
@@ -6492,12 +6487,6 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			       p);
 			break;
 		}
-
-		if (*p != ';' && *p != ',') {
-			/* End of param or invalid format */
-			break;
-		}
-		p++;
 	}
 out:
 	spin_unlock(&resource_alignment_lock);
-- 
2.51.0

[PATCH v10 4/6] PCI: Refactor disable_acs_redir and config_acs param handling

[Wei Wang]

The current implementation mixes disable_acs_redir and config_acs param
processing inside __pci_config_acs(), using acs_mask==0 as an implicit
signal to switch into config_acs_param mode. The intertwined logic is
hard to follow and easy to break:

  - The acs_mask==0 special case is non-obvious and obscures which code
    paths apply to which parameter.
  - The interleaved logic is fragile; changes intended for one parameter
    can unintentionally affect the other. For example,
    pci_dev_specific_disable_acs_redir() is invoked on the common path
    even though it should apply only to disable_acs_redir.

Split the two behaviors into dedicated functions,
pci_param_disable_acs_redir() and pci_param_config_acs(), making the
control flow explicit and easier to maintain.

The new pci_param_config_acs() implementation also improves on the
original logic:

  - It searches for the matching device first and parses ACS flags only
    for the matched device, avoiding unnecessary work.
  - A switch statement replaces multiple if/else blocks, improving
    readability.
  - Variable names (enabled_bits, disabled_bits) more clearly express
    their roles compared to the previous mask/flags pair.
  - Error messages are clearer.

This refactor separates concerns, improves robustness, and makes future
extensions to ACS parameter handling safer and easier to review.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
---
 drivers/pci/pci.c | 180 +++++++++++++++++++++++++++-------------------
 1 file changed, 108 insertions(+), 72 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3e2e34b24019..ffb489e4cbef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -912,90 +912,121 @@ struct pci_acs {
 	u16 fw_ctrl;
 };
 
-static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
-			     const char *p, const u16 acs_mask, const u16 acs_flags)
+static bool pci_dev_match_disable_acs_redir(struct pci_dev *dev, const char *p)
 {
-	u16 valid_ctrl = dev->acs_capabilities & PCI_ACS_BASIC_CTRL_MASK;
-	u16 flags = acs_flags;
-	u16 mask = acs_mask;
-	char *delimit;
-	int ret = 0;
-
-	if (!p)
-		return;
+	int ret;
 
 	while (*p) {
-		if (!acs_mask) {
-			/* Check for ACS flags */
-			delimit = strstr(p, "@");
-			if (delimit) {
-				int end;
-				u32 shift = 0;
-
-				end = delimit - p - 1;
-				mask = 0;
-				flags = 0;
-
-				while (end > -1) {
-					if (*(p + end) == '0') {
-						mask |= 1 << shift;
-						shift++;
-						end--;
-					} else if (*(p + end) == '1') {
-						mask |= 1 << shift;
-						flags |= 1 << shift;
-						shift++;
-						end--;
-					} else if ((*(p + end) == 'x') || (*(p + end) == 'X')) {
-						shift++;
-						end--;
-					} else {
-						pci_err(dev, "Invalid ACS flags... Ignoring\n");
-						return;
-					}
-				}
-				p = delimit + 1;
-			} else {
-				pci_err(dev, "ACS Flags missing\n");
-				return;
-			}
-		}
-
 		ret = pci_dev_str_match(dev, p, &p);
+		if (ret == 1)
+			return true;
 		if (ret < 0) {
-			pr_warn_once("PCI: Can't parse ACS command line parameter\n");
-			break;
-		} else if (ret == 1) {
-			/* Found a match */
+			pr_warn_once("PCI: Can't parse disable_acs_redir param\n");
 			break;
 		}
 	}
 
-	if (ret != 1)
+	return false;
+}
+
+static void pci_param_disable_acs_redir(struct pci_dev *dev,
+					struct pci_acs *caps)
+{
+	const u16 acs_redir_mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC;
+
+	if (!disable_acs_redir_param ||
+	    !pci_dev_match_disable_acs_redir(dev, disable_acs_redir_param) ||
+	    !pci_dev_specific_disable_acs_redir(dev))
 		return;
 
-	if (!pci_dev_specific_disable_acs_redir(dev))
-		return;
+	caps->ctrl = caps->fw_ctrl & ~acs_redir_mask;
+}
 
-	if (!acs_mask && (mask & ~valid_ctrl)) {
-		pci_warn(dev, "Ignoring unsupported ACS bits: %#06x\n",
-			 mask & ~valid_ctrl);
-		mask &= valid_ctrl;
-		flags &= valid_ctrl;
-	}
-
-	pci_dbg(dev, "ACS mask  = %#06x\n", mask);
-	pci_dbg(dev, "ACS flags = %#06x\n", flags);
-	pci_dbg(dev, "ACS control = %#06x\n", caps->ctrl);
-	pci_dbg(dev, "ACS fw_ctrl = %#06x\n", caps->fw_ctrl);
+static const char *pci_dev_match_config_acs(struct pci_dev *dev, const char *p)
+{
+	const char *seg;
+	int ret;
 
 	/*
-	 * For mask bits that are 0, copy them from the firmware setting
-	 * and apply flags for all the mask bits that are 1.
+	 * Example: 11101010x@0000:01:00.0;101x@0000:02:00.0;1@0000:03:00.0
+	 * Three segments delimited via ';'. @seg always points to the start
+	 * of a segment and p advances to the start of the device id (BDF)
+	 * after '@'. Upon match, return the start of the matching segment.
 	 */
-	caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
+	while (*p) {
+		seg = p;
+		p = strchr(seg, '@');
+		/* If malformed string, stop parsing. */
+		if (!p) {
+			pci_err(dev, "ACS Flags missing\n");
+			break;
+		}
 
-	pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
+		p++;
+		/* pci_dev_str_match() updates p to the next segment. */
+		ret = pci_dev_str_match(dev, p, &p);
+		if (ret == 1)
+			return seg;
+
+		if (ret < 0) {
+			pr_warn_once("PCI: Can't parse config_acs param\n");
+			break;
+		}
+	}
+
+	return NULL;
+}
+
+static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
+{
+	u16 shift = 0, enabled_bits = 0, disabled_bits = 0, invalid_bits;
+	u16 valid_ctrl = dev->acs_capabilities & PCI_ACS_BASIC_CTRL_MASK;
+	const char *p, *seg;
+
+	if (!config_acs_param || !valid_ctrl)
+		return;
+
+	seg = pci_dev_match_config_acs(dev, config_acs_param);
+	if (!seg)
+		return;
+
+	p = strchr(seg, '@');
+	/* Parse bitstring backwards from '@' */
+	while (p > seg) {
+		if (shift >= 16) {
+			pci_err(dev, "ACS bitstring exceeds 16 bits\n");
+			return;
+		}
+		switch (*--p) {
+		case '1':
+			enabled_bits |= BIT(shift);
+			break;
+		case '0':
+			disabled_bits |= BIT(shift);
+			break;
+		case 'x':
+		case 'X':
+			/* Skip this bit */
+			break;
+		default:
+			pci_err(dev, "Invalid ACS flag character '%c'\n", *p);
+			return;
+		}
+		shift++;
+	}
+
+	invalid_bits = (enabled_bits | disabled_bits) & ~valid_ctrl;
+	if (invalid_bits) {
+		pci_warn(dev, "Ignoring unsupported ACS bits: %#06x\n",
+			 invalid_bits);
+		enabled_bits &= valid_ctrl;
+		disabled_bits &= valid_ctrl;
+	}
+
+	pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n",
+		enabled_bits, disabled_bits);
+
+	caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
 }
 
 /**
@@ -1030,6 +1061,7 @@ void pci_enable_acs(struct pci_dev *dev)
 {
 	struct pci_acs caps;
 	bool enable_acs = false;
+	u16 kernel_default_ctrl;
 	int pos;
 
 	/* If an iommu is present we start with kernel default caps */
@@ -1048,14 +1080,18 @@ void pci_enable_acs(struct pci_dev *dev)
 	if (enable_acs)
 		pci_std_enable_acs(dev, &caps);
 
+	kernel_default_ctrl = caps.ctrl;
+
 	/*
 	 * Always apply caps from the command line, even if there is no iommu.
 	 * Trust that the admin has a reason to change the ACS settings.
 	 */
-	__pci_config_acs(dev, &caps, disable_acs_redir_param,
-			 PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC,
-			 ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC));
-	__pci_config_acs(dev, &caps, config_acs_param, 0, 0);
+	pci_param_disable_acs_redir(dev, &caps);
+	pci_param_config_acs(dev, &caps);
+
+	if (caps.ctrl != kernel_default_ctrl)
+		pci_info(dev, "User configured ACS to %#06x (default: %#06x)\n",
+			 caps.ctrl, kernel_default_ctrl);
 
 	pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
 }
-- 
2.51.0

[PATCH v10 5/6] PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

[Wei Wang]

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                             | 39 ++++++++++++++++++-
 include/uapi/linux/pci_regs.h                 | 13 +++++++
 3 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 8c4d3aa6fc67..6b5d884622d9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5331,13 +5331,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 ffb489e4cbef..4942bb6f1e11 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -41,6 +41,9 @@
 	(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | \
 	 PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)
 
+#define PCI_ACS_ECAP_CTRL_MASK \
+	(PCI_ACS_IB | PCI_ACS_DMAC_MASK | PCI_ACS_UMAC_MASK | PCI_ACS_URRC)
+
 DEFINE_MUTEX(pci_slot_mutex);
 
 const char *pci_power_names[] = {
@@ -982,6 +985,10 @@ static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
 	u16 shift = 0, enabled_bits = 0, disabled_bits = 0, invalid_bits;
 	u16 valid_ctrl = dev->acs_capabilities & PCI_ACS_BASIC_CTRL_MASK;
 	const char *p, *seg;
+	u16 new_ctrl;
+
+	if (dev->acs_capabilities & PCI_ACS_ECAP)
+		valid_ctrl |= PCI_ACS_ECAP_CTRL_MASK;
 
 	if (!config_acs_param || !valid_ctrl)
 		return;
@@ -1023,10 +1030,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;
 }
 
 /**
@@ -1048,6 +1068,23 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
 	/* Upstream Forwarding */
 	caps->ctrl |= (dev->acs_capabilities & PCI_ACS_UF);
 
+	/*
+	 * The ACS Enhanced Capability mechanisms are only defined for Root
+	 * Ports and Switch Downstream Ports, so PCI_ACS_ECAP is only expected
+	 * on those port types. Enable I/O Request Blocking and DSP Memory
+	 * Target Access Redirect on both; Unclaimed Request Redirect and USP
+	 * Memory Target Access Redirect apply to Switch Downstream Ports only.
+	 */
+	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..d09a1bf70117 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 */
-- 
2.51.0

[PATCH v10 6/6] PCI: Add the enhanced ACS controls check to pci_acs_flags_enabled()

[Wei Wang]

The enhanced ACS controls introduced by PCIe Gen 5 ensures better device
isolation. On devices that support the PCI_ACS_ECAP capability, the
controls are required to be enabled properly:
- ACS I/O Request Blocking needs to be enabled to avoid unintended
  upstream I/O requests.
- ACS DSP and USP Memory Target Access Control needs to be set with
  Request Redirect or Request Blocking to ensure the Downstream and
  Upstream Port memory resource ranges are not accessed by upstream
  memory requests.
- ACS Unclaimed Request Redirect needs to be enabled to ensure accesses to
  areas that lies within a Switch's Upstream Port memory apertures but not
  within any Downstream Port memory apertures get redirected.

To maintain compatibility with legacy devices that lack PCI_ACS_ECAP
support, pci_acs_enabled() skips checking for the capability.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/pci/pci.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4942bb6f1e11..89dd519cd311 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3652,6 +3652,57 @@ void pci_configure_ari(struct pci_dev *dev)
 	}
 }
 
+static bool pci_dev_has_memory_bars(struct pci_dev *pdev)
+{
+	int i;
+
+	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
+		if (pci_resource_flags(pdev, i) & IORESOURCE_MEM)
+			return true;
+	}
+
+	return false;
+}
+
+static bool pci_acs_ecap_enabled(struct pci_dev *pdev, u16 ctrl)
+{
+	struct pci_dev *upstream_pdev;
+
+	/*
+	 * For ACS DSP/USP Memory Target Access Control, either Request
+	 * Redirect or Request Blocking must be enabled to enforce isolation.
+	 * According to PCIe spec 7.0, the DSP Memory Target Access is
+	 * applicable to both Root Ports and Switch Downstream Ports that have
+	 * applicable Memory BAR space to protect. So if the device does not
+	 * have a Memory BAR, it skips the check.
+	 */
+	if (pci_dev_has_memory_bars(pdev) &&
+	    FIELD_GET(PCI_ACS_DMAC_MASK, ctrl) != PCI_ACS_MAC_RB &&
+	    FIELD_GET(PCI_ACS_DMAC_MASK, ctrl) != PCI_ACS_MAC_RR)
+		return false;
+
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
+		upstream_pdev = pci_upstream_bridge(pdev);
+
+		/*
+		 * The USP Memory Target Access is only applicable to
+		 * downstream ports that have applicable Memory BAR space in
+		 * the Switch Upstream Port to protect.
+		 */
+		if (upstream_pdev && pci_dev_has_memory_bars(upstream_pdev) &&
+		    FIELD_GET(PCI_ACS_UMAC_MASK, ctrl) != PCI_ACS_MAC_RB &&
+		    FIELD_GET(PCI_ACS_UMAC_MASK, ctrl) != PCI_ACS_MAC_RR)
+			return false;
+
+		/* PCI_ACS_URRC is applicable to Downstream Ports only. */
+		if (!(ctrl & PCI_ACS_URRC))
+			return false;
+	}
+
+	/* PCI_ACS_IB is applicable to both Root and Downstream Ports. */
+	return ctrl & PCI_ACS_IB;
+}
+
 static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
 {
 	int pos;
@@ -3669,6 +3720,18 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
 	acs_flags &= (pdev->acs_capabilities | PCI_ACS_EC);
 
 	pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
+
+	if (acs_flags & PCI_ACS_ECAP) {
+		if (!pci_acs_ecap_enabled(pdev, ctrl))
+			return false;
+		/*
+		 * The check for the required controls in PCI_ACS_ECAP has
+		 * passed. Clear the ECAP flag and continue to check the
+		 * basic ACS controls.
+		 */
+		acs_flags &= ~PCI_ACS_ECAP;
+	}
+
 	return (ctrl & acs_flags) == acs_flags;
 }
 
@@ -3727,6 +3790,8 @@ bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags)
 	 */
 	case PCI_EXP_TYPE_DOWNSTREAM:
 	case PCI_EXP_TYPE_ROOT_PORT:
+		/* PCI_ACS_ECAP applies to Root and Downstream Ports only */
+		acs_flags |= PCI_ACS_ECAP;
 		return pci_acs_flags_enabled(pdev, acs_flags);
 	/*
 	 * PCIe 3.0, 6.12.1.2 specifies ACS capabilities that should be
-- 
2.51.0

Re: [PATCH v10 3/6] PCI: Consolidate delimiter handling into pci_dev_str_match()

[sashiko-bot]

> Callers of pci_dev_str_match() manually checked for the ';' or ','
> delimiter and advanced the parameter pointer past it. Move this common
> logic into pci_dev_str_match() so callers no longer need to duplicate it.
> 
> As a side effect, pci_dev_str_match() now checks that a
> pci:<vendor>:<device>[:<subvendor>:<subdevice>] token is immediately
> followed by ';', ',' or the end of the string, and returns -EINVAL
> otherwise. Previously, trailing characters after such a token (e.g.,
> "pci=config_acs=110x@pci:10de:1234zz") were silently ignored: a
> matching token was accepted as a match, and a non-matching one quietly
> ended parsing of the rest of the parameter. Callers now report a parse
> error in both cases.
> 
> The [<domain>:]<bus>:<device>.<func> form is unaffected, since
> pci_dev_str_match_path() already rejects trailing characters.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB43935D2C01946E6D8FD9EFAADC832@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=3

Re: [PATCH v10 1/6] PCI: Validate ACS control bits against device-specific ACS capabilities

[sashiko-bot]

> The ACS control validation used the kernel's full set of ACS control bits,
> which allowed users to request ACS features that the device does not
> support. Because hardware silently ignores these unsupported bits, users
> have no indication that their config_acs= request was partially
> ineffective.
> 
> Validate the requested ACS control bits against dev->acs_capabilities so
> that only device-supported ACS controls are accepted. Mask the capability
> with PCI_ACS_BASIC_CTRL_MASK to select only the currently defined ACS
> control bits (0-6). Higher bits in the capability structure (e.g. the
> egress control vector size in bits 8-15) do not correspond to control bits
> in the ACS control register. Warn about any unsupported bits that are
> ignored.
> 
> __pci_config_acs() is also called from disable_acs_redir_param(), which
> passes a hardcoded acs_mask (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC) that
> does not need validation. Gate the new check on !acs_mask so it only
> fires for the config_acs_param path, which always invokes
> __pci_config_acs() with acs_mask == 0 and builds the actual mask from
> user input.
> 
> Also move the check after the device is matched, since the ctrl bits apply
> only to the matched device.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB43935D2C01946E6D8FD9EFAADC832@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=1

Re: [PATCH v10 4/6] PCI: Refactor disable_acs_redir and config_acs param handling

[sashiko-bot]

> The current implementation mixes disable_acs_redir and config_acs param
> processing inside __pci_config_acs(), using acs_mask==0 as an implicit
> signal to switch into config_acs_param mode. The intertwined logic is
> hard to follow and easy to break:
> 
>   - The acs_mask==0 special case is non-obvious and obscures which code
>     paths apply to which parameter.
>   - The interleaved logic is fragile; changes intended for one parameter
>     can unintentionally affect the other. For example,
>     pci_dev_specific_disable_acs_redir() is invoked on the common path
>     even though it should apply only to disable_acs_redir.
> 
> Split the two behaviors into dedicated functions,
> pci_param_disable_acs_redir() and pci_param_config_acs(), making the
> control flow explicit and easier to maintain.
> [ ... ]
> 
> This refactor separates concerns, improves robustness, and makes future
> extensions to ACS parameter handling safer and easier to review.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB43935D2C01946E6D8FD9EFAADC832@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=4

Re: [PATCH v10 2/6] Documentation/kernel-parameters: Add multi-device config_acs example

[sashiko-bot]

> The config_acs parameter allows configuring ACS settings for multiple PCI
> devices by separating entries with semicolons. The current documentation
> only illustrates applying one configuration pattern to all devices, but
> does not show how to specify multiple devices with different ACS settings
> in a single parameter. Add an example demonstrating multi-device usage
> with distinct ACS configurations.
> 
> Some bootloaders interpret ';' as a command separator, which can cause
> the parameter to be split as multiple commands. Document that the entire
> parameter may need to be quoted to avoid bootloader parsing issues. This
> avoids confusing bootloader errors such as "can't find command <BDF>"
> when passing multiple device entries.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB43935D2C01946E6D8FD9EFAADC832@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=2

Re: [PATCH v10 5/6] PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pci: missing port-type validation allows setting reserved UMAC/URRC bits on Root Ports
- [Medium] pci: unconditional check for reserved DMAC/UMAC encodings breaks config_acs on non-ECAP devices
--

commit d128af062fd915f74bfeafe2fdd8a9e6bffb5ab9
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 ffb489e4cbef6..4942bb6f1e111 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -982,6 +985,10 @@ static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
>  	u16 shift = 0, enabled_bits = 0, disabled_bits = 0, invalid_bits;
>  	u16 valid_ctrl = dev->acs_capabilities & PCI_ACS_BASIC_CTRL_MASK;
>  	const char *p, *seg;
> +	u16 new_ctrl;
> +
> +	if (dev->acs_capabilities & PCI_ACS_ECAP)
> +		valid_ctrl |= PCI_ACS_ECAP_CTRL_MASK;

[Severity: Medium]
Does this missing port-type validation allow setting reserved UMAC/URRC
bits on Root Ports?

Because PCI_ACS_ECAP_CTRL_MASK unconditionally includes the UMAC and URRC
bits, an administrator could use the config_acs= boot parameter to
overwrite these reserved bits on a Root Port device that supports
PCI_ACS_ECAP. 

The PCIe Base Specification states these bits must be hardwired to 0 for Root
Ports, so permitting them to be configured here could lead to undefined
behavior on poorly-behaved hardware.

>  
>  	if (!config_acs_param || !valid_ctrl)
>  		return;
[ ... ]
> @@ -1023,10 +1030,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) {

[Severity: Medium]
Does this unconditional check for reserved DMAC/UMAC encodings break
config_acs on non-ECAP devices?

If a non-ECAP device has firmware or hardware quirks that leave bits 8-11
initialized to 1s, this check will falsely trigger. Because it runs
unconditionally, it applies ECAP-specific validation constraints to non-ECAP
devices where the bits are just reserved. 

This will cause the function to abort, preventing the user from configuring
even basic, supported ACS controls on the device.

> +		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;
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB43935D2C01946E6D8FD9EFAADC832@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=5

Re: [PATCH v10 6/6] PCI: Add the enhanced ACS controls check to pci_acs_flags_enabled()

[sashiko-bot]

> The enhanced ACS controls introduced by PCIe Gen 5 ensures better device
> isolation. On devices that support the PCI_ACS_ECAP capability, the
> controls are required to be enabled properly:
> - ACS I/O Request Blocking needs to be enabled to avoid unintended
>   upstream I/O requests.
> - ACS DSP and USP Memory Target Access Control needs to be set with
>   Request Redirect or Request Blocking to ensure the Downstream and
>   Upstream Port memory resource ranges are not accessed by upstream
>   memory requests.
> - ACS Unclaimed Request Redirect needs to be enabled to ensure accesses to
>   areas that lies within a Switch's Upstream Port memory apertures but not
>   within any Downstream Port memory apertures get redirected.
> 
> To maintain compatibility with legacy devices that lack PCI_ACS_ECAP
> support, pci_acs_enabled() skips checking for the capability.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB43935D2C01946E6D8FD9EFAADC832@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=6