Re: [PATCH V1] PCI: Extend ACS configurability

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, May 08, 2024 at 04:56:58PM +0530, Vidya Sagar wrote:

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 41644336e..b4a8207eb 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4456,6 +4456,28 @@
>  				bridges without forcing it upstream. Note:
>  				this removes isolation between devices and
>  				may put more devices in an IOMMU group.
> +		config_acs=
> +				Format:
> +				=<ACS flags>@<pci_dev>[; ...]
> +				Specify one or more PCI devices (in the format
> +				specified above) optionally prepended with flags
> +				and separated by semicolons. The respective
> +				capabilities will be enabled, disabled or unchanged
> +				based on what is specified in 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
> +				Each bit can be marked as
> +				‘0‘ – force disabled
> +				‘1’ – force enabled
> +				‘x’ – unchanged.

It looks like 'x' doesn't fully work? Or at least it doesn't do what
I'd expect - preserve the FW setting of the bit.

> @@ -1005,6 +1076,7 @@ static void pci_enable_acs(struct pci_dev *dev)
>  	 * preferences.
>  	 */
>  	pci_disable_acs_redir(dev);
> +	pci_config_acs(dev);

Because this sequence starts with:

	pci_std_enable_acs(dev);

disable_acs_redir:
	pci_disable_acs_redir(dev);
	pci_config_acs(dev);

And pci_std_enable_acs() has already mangled the ACS flags:

	ctrl |= (cap & PCI_ACS_SV);
	ctrl |= (cap & PCI_ACS_RR);
	ctrl |= (cap & PCI_ACS_CR);
	ctrl |= (cap & PCI_ACS_UF);
	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
		ctrl |= (cap & PCI_ACS_TB);
	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);

So any FW setting on those bits is toast at this point.

It would be nicer if this code was structured a bit more robustly so
that it only wrote the ACS bits once after evaluating all the three
sources of configuration.

But I like the idea, I think this is a nice improvement.

Something sort of like this perhaps:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 94313641bc63fa..64b852ec3d613c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -948,12 +948,20 @@ void pci_request_acs(void)
 static const char *disable_acs_redir_param;
 static const char *config_acs_param;
 
-static void __pci_config_acs(struct pci_dev *dev, const char *p,
-			     u16 mask, u16 flags)
+struct pci_acs {
+	u16 cap;
+	u16 ctrl;
+	u16 fw_ctrl;
+};
+
+static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
+			     const char *p, u16 mask, u16 flags)
 {
 	char *delimit;
 	int ret = 0;
-	u16 ctrl, pos;
+
+	if (!p)
+		return;
 
 	while (*p) {
 		if (!mask) {
@@ -1018,98 +1026,37 @@ static void __pci_config_acs(struct pci_dev *dev, const char *p,
 	if (!pci_dev_specific_disable_acs_redir(dev))
 		return;
 
-	pos = dev->acs_cap;
-	if (!pos) {
-		pci_warn(dev, "cannot configure ACS for this hardware as it does not have ACS capabilities\n");
-		return;
-	}
-
 	pci_dbg(dev, "ACS mask  = 0x%X\n", mask);
 	pci_dbg(dev, "ACS flags = 0x%X\n", flags);
 
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-	ctrl &= ~mask;
-	ctrl |= flags;
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+	/* If mask is 0 then we copy the bit from the firmware setting. */
+	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
+	/* FIXME: flags doesn't check for supported? */
+	caps->ctrl |= flags;
 
-	pci_info(dev, "Configured ACS\n");
+	pci_info(dev, "Configured ACS to 0x%x\n", caps->ctrl);
 }
-
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
-{
-	const char *p;
-	u16 mask = 0, flags = 0;
-
-	if (!disable_acs_redir_param)
-		return;
-
-	p = disable_acs_redir_param;
-
-	mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC;
-	flags = ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
-
-	__pci_config_acs(dev, p, mask, flags);
-}
-
-/**
- * pci_config_acs - configure ACS capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the config_acs parameter.
- */
-static void pci_config_acs(struct pci_dev *dev)
-{
-	const char *p;
-	u16 mask = 0, flags = 0;
-
-	if (!config_acs_param)
-		return;
-
-	p = config_acs_param;
-
-	__pci_config_acs(dev, p, mask, flags);
-}
-
 /**
  * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
  * @dev: the PCI device
  */
-static void pci_std_enable_acs(struct pci_dev *dev)
+static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
 {
-	int pos;
-	u16 cap;
-	u16 ctrl;
-
-	pos = dev->acs_cap;
-	if (!pos)
-		return;
-
-	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-
 	/* Source Validation */
-	ctrl |= (cap & PCI_ACS_SV);
+	caps->ctrl |= (caps->cap & PCI_ACS_SV);
 
 	/* P2P Request Redirect */
-	ctrl |= (cap & PCI_ACS_RR);
+	caps->ctrl |= (caps->cap & PCI_ACS_RR);
 
 	/* P2P Completion Redirect */
-	ctrl |= (cap & PCI_ACS_CR);
+	caps->ctrl |= (caps->cap & PCI_ACS_CR);
 
 	/* Upstream Forwarding */
-	ctrl |= (cap & PCI_ACS_UF);
+	caps->ctrl |= (caps->cap & PCI_ACS_UF);
 
 	/* Enable Translation Blocking for external devices and noats */
 	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
-		ctrl |= (cap & PCI_ACS_TB);
-
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+		caps->ctrl |= (caps->cap & PCI_ACS_TB);
 }
 
 /**
@@ -1118,24 +1065,33 @@ static void pci_std_enable_acs(struct pci_dev *dev)
  */
 static void pci_enable_acs(struct pci_dev *dev)
 {
-	if (!pci_acs_enable)
-		goto disable_acs_redir;
+	struct pci_acs caps;
+	int pos;
 
-	if (!pci_dev_specific_enable_acs(dev))
-		goto disable_acs_redir;
+	pos = dev->acs_cap;
+	if (!pos)
+		return;
 
-	pci_std_enable_acs(dev);
+	pci_read_config_word(dev, pos + PCI_ACS_CAP, &caps.cap);
+	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
+	caps.fw_ctrl = caps.ctrl;
+
+	/* If an iommu is present we start with kernel default caps */
+	if (pci_acs_enable) {
+		if (pci_dev_specific_enable_acs(dev))
+			pci_std_enable_acs(dev, &caps);
+	}
 
-disable_acs_redir:
 	/*
-	 * Note: pci_disable_acs_redir() must be called even if ACS was not
-	 * enabled by the kernel because it may have been enabled by
-	 * platform firmware.  So if we are told to disable it, we should
-	 * always disable it after setting the kernel's default
-	 * preferences.
+	 * 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_disable_acs_redir(dev);
-	pci_config_acs(dev);
+	__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_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
 }
 
 /**