diff mbox series

[v2] PCI: Fix Extend ACS configurability

Message ID 20250123033716.112115-1-tdave@nvidia.com (mailing list archive)
State New
Headers show
Series [v2] PCI: Fix Extend ACS configurability | expand

Commit Message

Tushar Dave Jan. 23, 2025, 3:37 a.m. UTC
Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced
bugs that fail to configure ACS ctrl to the value specified by the
kernel parameter. Essentially there are two bugs.

First, when ACS is configured for multiple PCI devices using
'config_acs' kernel parameter, it results into error "PCI: Can't parse
ACS command line parameter". This is due to the bug in code that doesn't
preserve the ACS mask instead overwrites the mask with value 0.

For example, using 'config_acs' to configure ACS ctrl for multiple BDFs
fails:

	Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	PCI: Can't parse ACS command line parameter
	pci 0020:02:00.0: ACS mask  = 0x007f
	pci 0020:02:00.0: ACS flags = 0x007b
	pci 0020:02:00.0: Configured ACS to 0x007b

After this fix:

	Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	pci 0020:02:00.0: ACS mask  = 0x007f
	pci 0020:02:00.0: ACS flags = 0x007b
	pci 0020:02:00.0: ACS control = 0x005f
	pci 0020:02:00.0: ACS fw_ctrl = 0x0053
	pci 0020:02:00.0: Configured ACS to 0x007b
	pci 0039:00:00.0: ACS mask  = 0x0070
	pci 0039:00:00.0: ACS flags = 0x0050
	pci 0039:00:00.0: ACS control = 0x001d
	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
	pci 0039:00:00.0: Configured ACS to 0x0050

Second bug is in the bit manipulation logic where we copy the bit from
the firmware settings when mask bit 0.

For example, 'disable_acs_redir' fails to clear all three ACS P2P redir
bits due to the wrong bit fiddling:

	Kernel command line: pci=disable_acs_redir=0020:02:00.0;0030:02:00.0;0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	pci 0020:02:00.0: ACS mask  = 0x002c
	pci 0020:02:00.0: ACS flags = 0xffd3
	pci 0020:02:00.0: Configured ACS to 0xfffb
	pci 0030:02:00.0: ACS mask  = 0x002c
	pci 0030:02:00.0: ACS flags = 0xffd3
	pci 0030:02:00.0: Configured ACS to 0xffdf
	pci 0039:00:00.0: ACS mask  = 0x002c
	pci 0039:00:00.0: ACS flags = 0xffd3
	pci 0039:00:00.0: Configured ACS to 0xffd3

After this fix:

	Kernel command line: pci=disable_acs_redir=0020:02:00.0;0030:02:00.0;0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	pci 0020:02:00.0: ACS mask  = 0x002c
	pci 0020:02:00.0: ACS flags = 0xffd3
	pci 0020:02:00.0: ACS control = 0x007f
	pci 0020:02:00.0: ACS fw_ctrl = 0x007b
	pci 0020:02:00.0: Configured ACS to 0x0053
	pci 0030:02:00.0: ACS mask  = 0x002c
	pci 0030:02:00.0: ACS flags = 0xffd3
	pci 0030:02:00.0: ACS control = 0x005f
	pci 0030:02:00.0: ACS fw_ctrl = 0x005f
	pci 0030:02:00.0: Configured ACS to 0x0053
	pci 0039:00:00.0: ACS mask  = 0x002c
	pci 0039:00:00.0: ACS flags = 0xffd3
	pci 0039:00:00.0: ACS control = 0x001d
	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
	pci 0039:00:00.0: Configured ACS to 0x0000

Fixes: 47c8846a49ba ("PCI: Extend ACS configurability")
Signed-off-by: Tushar Dave <tdave@nvidia.com>
---

changes in v2:
 - Addressed review comments by Jason and Bjorn.
 - Removed Documentation changes (already taken care by other patch).
 - Amended commit description.

 drivers/pci/pci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Jason Gunthorpe Jan. 23, 2025, 1:35 p.m. UTC | #1
On Wed, Jan 22, 2025 at 07:37:16PM -0800, Tushar Dave wrote:
> Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced
> bugs that fail to configure ACS ctrl to the value specified by the
> kernel parameter. Essentially there are two bugs.

>  drivers/pci/pci.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

You will probably need to rebase and resend this once the merge window
closes (Feb 3) but it looks OK now

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Sathyanarayanan Kuppuswamy Jan. 23, 2025, 10:24 p.m. UTC | #2
Hi,

On 1/22/25 7:37 PM, Tushar Dave wrote:
> Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced
> bugs that fail to configure ACS ctrl to the value specified by the
> kernel parameter. Essentially there are two bugs.
>
> First, when ACS is configured for multiple PCI devices using
> 'config_acs' kernel parameter, it results into error "PCI: Can't parse
> ACS command line parameter". This is due to the bug in code that doesn't
> preserve the ACS mask instead overwrites the mask with value 0.
>
> For example, using 'config_acs' to configure ACS ctrl for multiple BDFs
> fails:
>
> 	Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
> 	PCI: Can't parse ACS command line parameter
> 	pci 0020:02:00.0: ACS mask  = 0x007f
> 	pci 0020:02:00.0: ACS flags = 0x007b
> 	pci 0020:02:00.0: Configured ACS to 0x007b
>
> After this fix:
>
> 	Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
> 	pci 0020:02:00.0: ACS mask  = 0x007f
> 	pci 0020:02:00.0: ACS flags = 0x007b
> 	pci 0020:02:00.0: ACS control = 0x005f
> 	pci 0020:02:00.0: ACS fw_ctrl = 0x0053
> 	pci 0020:02:00.0: Configured ACS to 0x007b
> 	pci 0039:00:00.0: ACS mask  = 0x0070
> 	pci 0039:00:00.0: ACS flags = 0x0050
> 	pci 0039:00:00.0: ACS control = 0x001d
> 	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
> 	pci 0039:00:00.0: Configured ACS to 0x0050
>
> Second bug is in the bit manipulation logic where we copy the bit from
> the firmware settings when mask bit 0.
>
> For example, 'disable_acs_redir' fails to clear all three ACS P2P redir
> bits due to the wrong bit fiddling:
>
> 	Kernel command line: pci=disable_acs_redir=0020:02:00.0;0030:02:00.0;0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
> 	pci 0020:02:00.0: ACS mask  = 0x002c
> 	pci 0020:02:00.0: ACS flags = 0xffd3
> 	pci 0020:02:00.0: Configured ACS to 0xfffb
> 	pci 0030:02:00.0: ACS mask  = 0x002c
> 	pci 0030:02:00.0: ACS flags = 0xffd3
> 	pci 0030:02:00.0: Configured ACS to 0xffdf
> 	pci 0039:00:00.0: ACS mask  = 0x002c
> 	pci 0039:00:00.0: ACS flags = 0xffd3
> 	pci 0039:00:00.0: Configured ACS to 0xffd3
>
> After this fix:
>
> 	Kernel command line: pci=disable_acs_redir=0020:02:00.0;0030:02:00.0;0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
> 	pci 0020:02:00.0: ACS mask  = 0x002c
> 	pci 0020:02:00.0: ACS flags = 0xffd3
> 	pci 0020:02:00.0: ACS control = 0x007f
> 	pci 0020:02:00.0: ACS fw_ctrl = 0x007b
> 	pci 0020:02:00.0: Configured ACS to 0x0053
> 	pci 0030:02:00.0: ACS mask  = 0x002c
> 	pci 0030:02:00.0: ACS flags = 0xffd3
> 	pci 0030:02:00.0: ACS control = 0x005f
> 	pci 0030:02:00.0: ACS fw_ctrl = 0x005f
> 	pci 0030:02:00.0: Configured ACS to 0x0053
> 	pci 0039:00:00.0: ACS mask  = 0x002c
> 	pci 0039:00:00.0: ACS flags = 0xffd3
> 	pci 0039:00:00.0: ACS control = 0x001d
> 	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
> 	pci 0039:00:00.0: Configured ACS to 0x0000
>
> Fixes: 47c8846a49ba ("PCI: Extend ACS configurability")
> Signed-off-by: Tushar Dave <tdave@nvidia.com>
> ---

LGTM

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
> changes in v2:
>   - Addressed review comments by Jason and Bjorn.
>   - Removed Documentation changes (already taken care by other patch).
>   - Amended commit description.
>
>   drivers/pci/pci.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0b29ec6e8e5e..19fbdd8643bc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -955,8 +955,10 @@ struct pci_acs {
>   };
>   
>   static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> -			     const char *p, u16 mask, u16 flags)
> +			     const char *p, const u16 acs_mask, const u16 acs_flags)
>   {
> +	u16 flags = acs_flags;
> +	u16 mask = acs_mask;
>   	char *delimit;
>   	int ret = 0;
>   
> @@ -964,7 +966,7 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>   		return;
>   
>   	while (*p) {
> -		if (!mask) {
> +		if (!acs_mask) {
>   			/* Check for ACS flags */
>   			delimit = strstr(p, "@");
>   			if (delimit) {
> @@ -972,6 +974,8 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>   				u32 shift = 0;
>   
>   				end = delimit - p - 1;
> +				mask = 0;
> +				flags = 0;
>   
>   				while (end > -1) {
>   					if (*(p + end) == '0') {
> @@ -1028,10 +1032,13 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>   
>   	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);
>   
> -	/* If mask is 0 then we copy the bit from the firmware setting. */
> -	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
> -	caps->ctrl |= flags;
> +	/* For mask bits that are 0 copy them from the firmware setting
> +	 * and apply flags for all the mask bits that are 1.
> +	 */
> +	caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
>   
>   	pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
>   }
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0b29ec6e8e5e..19fbdd8643bc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -955,8 +955,10 @@  struct pci_acs {
 };
 
 static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
-			     const char *p, u16 mask, u16 flags)
+			     const char *p, const u16 acs_mask, const u16 acs_flags)
 {
+	u16 flags = acs_flags;
+	u16 mask = acs_mask;
 	char *delimit;
 	int ret = 0;
 
@@ -964,7 +966,7 @@  static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 		return;
 
 	while (*p) {
-		if (!mask) {
+		if (!acs_mask) {
 			/* Check for ACS flags */
 			delimit = strstr(p, "@");
 			if (delimit) {
@@ -972,6 +974,8 @@  static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 				u32 shift = 0;
 
 				end = delimit - p - 1;
+				mask = 0;
+				flags = 0;
 
 				while (end > -1) {
 					if (*(p + end) == '0') {
@@ -1028,10 +1032,13 @@  static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 
 	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);
 
-	/* If mask is 0 then we copy the bit from the firmware setting. */
-	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
-	caps->ctrl |= flags;
+	/* For mask bits that are 0 copy them from the firmware setting
+	 * and apply flags for all the mask bits that are 1.
+	 */
+	caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
 
 	pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
 }