[PATCH 1/1] PCI: Fix Extend ACS configurability

[PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced a
bug that fails to configure ACS ctrl for multiple PCI devices. It
affects both 'config_acs' and 'disable_acs_redir'.

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

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

After this fix:

[    0.000000] Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p" earlycon
[   19.583814] pci 0020:02:00.0: ACS mask  = 0x007f
[   19.588532] pci 0020:02:00.0: ACS flags = 0x007b
[   19.593249] pci 0020:02:00.0: ACS control = 0x001d
[   19.598143] pci 0020:02:00.0: Configured ACS to 0x007b
[   24.088699] pci 0039:00:00.0: ACS mask  = 0x0070
[   24.093416] pci 0039:00:00.0: ACS flags = 0x0050
[   24.098136] pci 0039:00:00.0: ACS control = 0x001d
[   24.103031] pci 0039:00:00.0: Configured ACS to 0x005d

For example, using 'disable_acs_redire' fails to clear all three ACS P2P
redir bits:

[    0.000000] 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" earlycon
[   19.615860] pci 0020:02:00.0: ACS mask  = 0x002c
[   19.615862] pci 0020:02:00.0: ACS flags = 0xffd3
[   19.615863] pci 0020:02:00.0: Configured ACS to 0xfffb
[   22.332683] pci 0030:02:00.0: ACS mask  = 0x002c
[   22.332685] pci 0030:02:00.0: ACS flags = 0xffd3
[   22.332686] pci 0030:02:00.0: Configured ACS to 0xffdf
[   24.110278] pci 0039:00:00.0: ACS mask  = 0x002c
[   24.110281] pci 0039:00:00.0: ACS flags = 0xffd3
[   24.110283] pci 0039:00:00.0: Configured ACS to 0xffd3

After this fix:

[    0.000000] 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" earlycon
[   19.597909] pci 0020:02:00.0: ACS mask  = 0x002c
[   19.597910] pci 0020:02:00.0: ACS flags = 0xffd3
[   19.597911] pci 0020:02:00.0: ACS control = 0x007f
[   19.597911] pci 0020:02:00.0: Configured ACS to 0x0053
[   22.314124] pci 0030:02:00.0: ACS mask  = 0x002c
[   22.314126] pci 0030:02:00.0: ACS flags = 0xffd3
[   22.314127] pci 0030:02:00.0: ACS control = 0x005f
[   22.314128] pci 0030:02:00.0: Configured ACS to 0x0053
[   24.091711] pci 0039:00:00.0: ACS mask  = 0x002c
[   24.091712] pci 0039:00:00.0: ACS flags = 0xffd3
[   24.091714] pci 0039:00:00.0: ACS control = 0x001d
[   24.091715] pci 0039:00:00.0: Configured ACS to 0x0011

Fixes: 47c8846a49ba ("PCI: Extend ACS configurability")
Signed-off-by: Tushar Dave <tdave@nvidia.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 11 +++++------
 drivers/pci/pci.c                               | 16 +++++++++-------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index dc663c0ca670..fc1c37910d1c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4654,11 +4654,10 @@
 				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.
+				specified above) 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
@@ -4673,7 +4672,7 @@
 				  '1' – force enabled
 				  'x' – unchanged
 				For example,
-				  pci=config_acs=10x
+				  pci=config_acs=10x@pci:0:0
 				would configure all devices that support
 				ACS to enable P2P Request Redirect, disable
 				Translation Blocking, and leave Source
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0b29ec6e8e5e..35ff21b014ac 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -951,12 +951,13 @@ static const char *config_acs_param;
 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)
+			     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 +965,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 +973,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 +1031,10 @@ 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);
 
-	/* 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;
+	caps->ctrl &= ~mask;
+	caps->ctrl |= (flags & mask);
 
 	pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
 }
@@ -1082,7 +1085,6 @@ static void pci_enable_acs(struct pci_dev *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 (enable_acs)
 		pci_std_enable_acs(dev, &caps);
-- 
2.34.1

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Vidya Sagar]

Thanks Tushar for the fix.

Tested-by: Vidya Sagar <vidyas@nvidia.com>

________________________________________
From: Tushar Dave <tdave@nvidia.com>
Sent: Saturday, December 14, 2024 01:59
To: corbet@lwn.net <corbet@lwn.net>; bhelgaas@google.com <bhelgaas@google.com>; paulmck@kernel.org <paulmck@kernel.org>; akpm@linux-foundation.org <akpm@linux-foundation.org>; thuth@redhat.com <thuth@redhat.com>; rostedt@goodmis.org <rostedt@goodmis.org>; xiongwei.song@windriver.com <xiongwei.song@windriver.com>; Vidya Sagar <vidyas@nvidia.com>; linux-doc@vger.kernel.org <linux-doc@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-pci@vger.kernel.org <linux-pci@vger.kernel.org>
Cc: Vikram Sethi <vsethi@nvidia.com>; Jason Gunthorpe <jgg@nvidia.com>; Shanker Donthineni <sdonthineni@nvidia.com>; Tushar Dave <tdave@nvidia.com>
Subject: [PATCH 1/1] PCI: Fix Extend ACS configurability
 
Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced a
bug that fails to configure ACS ctrl for multiple PCI devices. It
affects both 'config_acs' and 'disable_acs_redir'.

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

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

After this fix:

[    0.000000] Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p" earlycon
[   19.583814] pci 0020:02:00.0: ACS mask  = 0x007f
[   19.588532] pci 0020:02:00.0: ACS flags = 0x007b
[   19.593249] pci 0020:02:00.0: ACS control = 0x001d
[   19.598143] pci 0020:02:00.0: Configured ACS to 0x007b
[   24.088699] pci 0039:00:00.0: ACS mask  = 0x0070
[   24.093416] pci 0039:00:00.0: ACS flags = 0x0050
[   24.098136] pci 0039:00:00.0: ACS control = 0x001d
[   24.103031] pci 0039:00:00.0: Configured ACS to 0x005d

For example, using 'disable_acs_redire' fails to clear all three ACS P2P
redir bits:

[    0.000000] 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" earlycon
[   19.615860] pci 0020:02:00.0: ACS mask  = 0x002c
[   19.615862] pci 0020:02:00.0: ACS flags = 0xffd3
[   19.615863] pci 0020:02:00.0: Configured ACS to 0xfffb
[   22.332683] pci 0030:02:00.0: ACS mask  = 0x002c
[   22.332685] pci 0030:02:00.0: ACS flags = 0xffd3
[   22.332686] pci 0030:02:00.0: Configured ACS to 0xffdf
[   24.110278] pci 0039:00:00.0: ACS mask  = 0x002c
[   24.110281] pci 0039:00:00.0: ACS flags = 0xffd3
[   24.110283] pci 0039:00:00.0: Configured ACS to 0xffd3

After this fix:

[    0.000000] 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" earlycon
[   19.597909] pci 0020:02:00.0: ACS mask  = 0x002c
[   19.597910] pci 0020:02:00.0: ACS flags = 0xffd3
[   19.597911] pci 0020:02:00.0: ACS control = 0x007f
[   19.597911] pci 0020:02:00.0: Configured ACS to 0x0053
[   22.314124] pci 0030:02:00.0: ACS mask  = 0x002c
[   22.314126] pci 0030:02:00.0: ACS flags = 0xffd3
[   22.314127] pci 0030:02:00.0: ACS control = 0x005f
[   22.314128] pci 0030:02:00.0: Configured ACS to 0x0053
[   24.091711] pci 0039:00:00.0: ACS mask  = 0x002c
[   24.091712] pci 0039:00:00.0: ACS flags = 0xffd3
[   24.091714] pci 0039:00:00.0: ACS control = 0x001d
[   24.091715] pci 0039:00:00.0: Configured ACS to 0x0011

Fixes: 47c8846a49ba ("PCI: Extend ACS configurability")
Signed-off-by: Tushar Dave <tdave@nvidia.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 11 +++++------
 drivers/pci/pci.c                               | 16 +++++++++-------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index dc663c0ca670..fc1c37910d1c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4654,11 +4654,10 @@
                                 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.
+                               specified above) 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
@@ -4673,7 +4672,7 @@
                                   '1' – force enabled
                                   'x' – unchanged
                                 For example,
-                                 pci=config_acs=10x
+                                 pci=config_acs=10x@pci:0:0
                                 would configure all devices that support
                                 ACS to enable P2P Request Redirect, disable
                                 Translation Blocking, and leave Source
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0b29ec6e8e5e..35ff21b014ac 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -951,12 +951,13 @@ static const char *config_acs_param;
 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)
+                            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 +965,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 +973,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 +1031,10 @@ 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);
 
-       /* 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;
+       caps->ctrl &= ~mask;
+       caps->ctrl |= (flags & mask);
 
         pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
 }
@@ -1082,7 +1085,6 @@ static void pci_enable_acs(struct pci_dev *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 (enable_acs)
                 pci_std_enable_acs(dev, &caps);
--
2.34.1

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe]

On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index dc663c0ca670..fc1c37910d1c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4654,11 +4654,10 @@
>  				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.
> +				specified above) 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
> @@ -4673,7 +4672,7 @@
>  				  '1' – force enabled
>  				  'x' – unchanged
>  				For example,
> -				  pci=config_acs=10x
> +				  pci=config_acs=10x@pci:0:0
>  				would configure all devices that support
>  				ACS to enable P2P Request Redirect, disable
>  				Translation Blocking, and leave Source

Is this an unrelated change? The format of the command line shouldn't
be changed to fix the described bug, why is the documentation changed?

>  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 +965,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 +973,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') {

This function the entire fix, right? Because the routine was
clobbering acs_mask as it processed the earlier devices?

> @@ -1028,10 +1031,10 @@ 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);
>  
> -	/* 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;
> +	caps->ctrl &= ~mask;
> +	caps->ctrl |= (flags & mask);

And why delete fw_ctrl? Doesn't that break the unchanged
functionality?

Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Bjorn Helgaas]

On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
> Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced a
> bug that fails to configure ACS ctrl for multiple PCI devices. It
> affects both 'config_acs' and 'disable_acs_redir'.
> 
> For example, using 'config_acs' to configure ACS ctrl for multiple BDFs
> fails:
> 
> [    0.000000] Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p" earlycon
> [   12.349875] PCI: Can't parse ACS command line parameter
> [   19.629314] pci 0020:02:00.0: ACS mask  = 0x007f
> [   19.629315] pci 0020:02:00.0: ACS flags = 0x007b
> [   19.629316] pci 0020:02:00.0: Configured ACS to 0x007b

Drop timestamps (unless they contribute to understanding the
problem) and indent the quoted material by a couple spaces.

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/2/25 10:40, Jason Gunthorpe wrote:
> On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index dc663c0ca670..fc1c37910d1c 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4654,11 +4654,10 @@
>>   				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.
>> +				specified above) 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
>> @@ -4673,7 +4672,7 @@
>>   				  '1' – force enabled
>>   				  'x' – unchanged
>>   				For example,
>> -				  pci=config_acs=10x
>> +				  pci=config_acs=10x@pci:0:0
>>   				would configure all devices that support
>>   				ACS to enable P2P Request Redirect, disable
>>   				Translation Blocking, and leave Source
> 
> Is this an unrelated change? The format of the command line shouldn't
> be changed to fix the described bug, why is the documentation changed?

The documentation as it is (i.e. without my patch), is not correct.

IOW, config_acs parameter does require flags and it is not optional. Without 
flags it results into "ACS Flags missing". Therefore I remove word "optionally" 
from the documentation text.

Secondly, the syntax in the example 'pci=config_acs=10x' is incorrect. The 
correct syntax should be 'pci=config_acs=10x@pci:0:0' that would configure all 
devices that support ACS to enable P2P Request Redirect, disable Translation 
Blocking, and leave Source Validation unchanged from whatever power-up or 
firmware set it to.

> 
>>   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 +965,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 +973,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') {
> 
> This function the entire fix, right? Because the routine was
> clobbering acs_mask as it processed the earlier devices?

yes, that is correct.

> 
>> @@ -1028,10 +1031,10 @@ 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);
>>   
>> -	/* 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;
>> +	caps->ctrl &= ~mask;
>> +	caps->ctrl |= (flags & mask);
> 
> And why delete fw_ctrl? Doesn't that break the unchanged
> functionality?

No, it does not break the unchanged functionality. I removed it because it is 
not needed after my fix.

If it helps, using 'config_acs' the code only allows to configures the lower 7 
bits of ACS ctrl for the specified PCI device(s).
The bits other than the lower 7 bits of ACS ctrl remain unchanged.
The bits specified with 'x' or 'X' that are within the 7 lower bits remain 
unchanged. Trying to configure bits other than lower 7 bits generates an error 
message "Invalid ACS flags specified"

-Tushar
> 
> Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/2/25 15:26, Bjorn Helgaas wrote:
> On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
>> Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced a
>> bug that fails to configure ACS ctrl for multiple PCI devices. It
>> affects both 'config_acs' and 'disable_acs_redir'.
>>
>> For example, using 'config_acs' to configure ACS ctrl for multiple BDFs
>> fails:
>>
>> [    0.000000] Kernel command line: pci=config_acs=1111011@0020:02:00.0;101xxxx@0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p" earlycon
>> [   12.349875] PCI: Can't parse ACS command line parameter
>> [   19.629314] pci 0020:02:00.0: ACS mask  = 0x007f
>> [   19.629315] pci 0020:02:00.0: ACS flags = 0x007b
>> [   19.629316] pci 0020:02:00.0: Configured ACS to 0x007b
> 
> Drop timestamps (unless they contribute to understanding the
> problem) and indent the quoted material by a couple spaces.
Thanks Bjorn. will do in v2.

-Tushar

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Bjorn Helgaas]

On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:
> On 1/2/25 10:40, Jason Gunthorpe wrote:
> > On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
> > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index dc663c0ca670..fc1c37910d1c 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -4654,11 +4654,10 @@
> > >   				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.
> > > +				specified above) 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
> > > @@ -4673,7 +4672,7 @@
> > >   				  '1' – force enabled
> > >   				  'x' – unchanged
> > >   				For example,
> > > -				  pci=config_acs=10x
> > > +				  pci=config_acs=10x@pci:0:0
> > >   				would configure all devices that support
> > >   				ACS to enable P2P Request Redirect, disable
> > >   				Translation Blocking, and leave Source
> > 
> > Is this an unrelated change? The format of the command line shouldn't
> > be changed to fix the described bug, why is the documentation changed?
> 
> The documentation as it is (i.e. without my patch), is not correct.
> 
> IOW, config_acs parameter does require flags and it is not optional. Without
> flags it results into "ACS Flags missing". Therefore I remove word
> "optionally" from the documentation text.
> 
> Secondly, the syntax in the example 'pci=config_acs=10x' is incorrect. The
> correct syntax should be 'pci=config_acs=10x@pci:0:0' that would configure
> all devices that support ACS to enable P2P Request Redirect, disable
> Translation Blocking, and leave Source Validation unchanged from whatever
> power-up or firmware set it to.

I'd suggest a separate patch to fix the documentation so we don't try
to relate the doc changes with the code changes.

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe]

On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:

> > > @@ -1028,10 +1031,10 @@ 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);
> > > -	/* 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;
> > > +	caps->ctrl &= ~mask;
> > > +	caps->ctrl |= (flags & mask);
> > 
> > And why delete fw_ctrl? Doesn't that break the unchanged
> > functionality?
> 
> No, it does not break the unchanged functionality. I removed it because it
> is not needed after my fix.

I mean, the whole hunk above is not needed, right? Or at least I don't
see how it relates to your commit message..

> If it helps, using 'config_acs' the code only allows to configures the lower
> 7 bits of ACS ctrl for the specified PCI device(s).
> The bits other than the lower 7 bits of ACS ctrl remain unchanged.
> The bits specified with 'x' or 'X' that are within the 7 lower bits remain
> unchanged. Trying to configure bits other than lower 7 bits generates an
> error message "Invalid ACS flags specified"

But the fw_ctrl was how the x behavior was supposed to be implemented,
IIRC there were cases where you could not just rely on caps->ctrl as
something prior had alreaded changed it.

What about your fix to the mask changes this reasoning? If nothing
then it should be its own patch with its own explanation..

Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/6/25 16:10, Jason Gunthorpe wrote:
> On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:
> 
>>>> @@ -1028,10 +1031,10 @@ 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);
>>>> -	/* 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;
>>>> +	caps->ctrl &= ~mask;
>>>> +	caps->ctrl |= (flags & mask);
>>>
>>> And why delete fw_ctrl? Doesn't that break the unchanged
>>> functionality?
>>
>> No, it does not break the unchanged functionality. I removed it because it
>> is not needed after my fix.
> 
> I mean, the whole hunk above is not needed, right? Or at least I don't
> see how it relates to your commit message..

Without the above hunk, there are cases where ACS flags do not get set 
correctly. Here is the example test case without above hunk in my patch (IOW 
keeping fw_ctrl changes as it is like original code plus pci_dbg to print debug 
info)


Kernel command line: pci=config_acs=xxx1010@pci:15b3:1979;1111@pci:10de:22b1

	pci 0000:02:00.0: ACS mask  = 0x000f
	pci 0000:02:00.0: ACS flags = 0x000a
	pci 0000:02:00.0: ACS control = 0x007f
	pci 0000:02:00.0: ACS fw_ctrl = 0x007f
	pci 0000:02:00.0: Configured ACS to 0x007f
	...
	..
	.
	pci 0039:00:00.0: ACS mask  = 0x000f
	pci 0039:00:00.0: ACS flags = 0x000f
	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 0x001f

As seen in the above output, ACS bits for BDF 0000:02:00.0 did not get set as 
expected. ACS ctrl for BDF 0000:02:00.0 should be 0x7a and not 0x7f.

> 
>> If it helps, using 'config_acs' the code only allows to configures the lower
>> 7 bits of ACS ctrl for the specified PCI device(s).
>> The bits other than the lower 7 bits of ACS ctrl remain unchanged.
>> The bits specified with 'x' or 'X' that are within the 7 lower bits remain
>> unchanged. Trying to configure bits other than lower 7 bits generates an
>> error message "Invalid ACS flags specified"
> 
> But the fw_ctrl was how the x behavior was supposed to be implemented,
> IIRC there were cases where you could not just rely on caps->ctrl as
> something prior had alreaded changed it.

I read your comment in https://lore.kernel.org/all/20240508131427.GH4650@nvidia.com/

Looking at the current code, the sequence begin with function 'pci_enable_acs()' 
that reads PCI_ACS_CTRL into caps->ctrl and invoke the below three functions 
that prepares caps->ctrl before writing to ACS CTRL reg.

	i.e.
	pci_std_enable_acs()
	__pci_config_acs(dev, &caps,disable_acs_redir_param,...)
	__pci_config_acs(dev, &caps, config_acs_param, 0, 0)

Here kernel command line takes precedence over 'pci_std_enable_acs()'.
'config_acs' takes precedence over 'disable_acs_redir'. IOW, if config_acs param 
is used then it takes the final control over what value is getting written to 
ACS CTRL reg and that is how it should be, no?

> 
> What about your fix to the mask changes this reasoning? If nothing
> then it should be its own patch with its own explanation..

Sure. As soon as we are align I'll take care of it.

> 
> Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/6/25 12:53, Bjorn Helgaas wrote:
> On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:
>> On 1/2/25 10:40, Jason Gunthorpe wrote:
>>> On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>> index dc663c0ca670..fc1c37910d1c 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -4654,11 +4654,10 @@
>>>>    				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.
>>>> +				specified above) 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
>>>> @@ -4673,7 +4672,7 @@
>>>>    				  '1' – force enabled
>>>>    				  'x' – unchanged
>>>>    				For example,
>>>> -				  pci=config_acs=10x
>>>> +				  pci=config_acs=10x@pci:0:0
>>>>    				would configure all devices that support
>>>>    				ACS to enable P2P Request Redirect, disable
>>>>    				Translation Blocking, and leave Source
>>>
>>> Is this an unrelated change? The format of the command line shouldn't
>>> be changed to fix the described bug, why is the documentation changed?
>>
>> The documentation as it is (i.e. without my patch), is not correct.
>>
>> IOW, config_acs parameter does require flags and it is not optional. Without
>> flags it results into "ACS Flags missing". Therefore I remove word
>> "optionally" from the documentation text.
>>
>> Secondly, the syntax in the example 'pci=config_acs=10x' is incorrect. The
>> correct syntax should be 'pci=config_acs=10x@pci:0:0' that would configure
>> all devices that support ACS to enable P2P Request Redirect, disable
>> Translation Blocking, and leave Source Validation unchanged from whatever
>> power-up or firmware set it to.
> 
> I'd suggest a separate patch to fix the documentation so we don't try
> to relate the doc changes with the code changes.

Sure thing. I will send a separate patch for the doc changes.

-Tushar

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe]

On Tue, Jan 07, 2025 at 06:32:42PM -0800, Tushar Dave wrote:
> 
> 
> On 1/6/25 16:10, Jason Gunthorpe wrote:
> > On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:
> > 
> > > > > @@ -1028,10 +1031,10 @@ 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);
> > > > > -	/* 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;
> > > > > +	caps->ctrl &= ~mask;
> > > > > +	caps->ctrl |= (flags & mask);
> > > > 
> > > > And why delete fw_ctrl? Doesn't that break the unchanged
> > > > functionality?
> > > 
> > > No, it does not break the unchanged functionality. I removed it because it
> > > is not needed after my fix.
> > 
> > I mean, the whole hunk above is not needed, right? Or at least I don't
> > see how it relates to your commit message..
> 
> Without the above hunk, there are cases where ACS flags do not get set
> correctly. Here is the example test case without above hunk in my patch (IOW
> keeping fw_ctrl changes as it is like original code plus pci_dbg to print
> debug info)

Isn't that because the bit logic in the code is wrong?

> -	/* If mask is 0 then we copy the bit from the firmware setting. */
> -	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);

That comment doesn't match the calculation at all.

> > > If it helps, using 'config_acs' the code only allows to configures the lower
> > > 7 bits of ACS ctrl for the specified PCI device(s).
> > > The bits other than the lower 7 bits of ACS ctrl remain unchanged.
> > > The bits specified with 'x' or 'X' that are within the 7 lower bits remain
> > > unchanged. Trying to configure bits other than lower 7 bits generates an
> > > error message "Invalid ACS flags specified"
> > 
> > But the fw_ctrl was how the x behavior was supposed to be implemented,
> > IIRC there were cases where you could not just rely on caps->ctrl as
> > something prior had alreaded changed it.
> 
> I read your comment in https://lore.kernel.org/all/20240508131427.GH4650@nvidia.com/
> 
> Looking at the current code, the sequence begin with function
> 'pci_enable_acs()' that reads PCI_ACS_CTRL into caps->ctrl and invoke the
> below three functions that prepares caps->ctrl before writing to ACS CTRL
> reg.

caps->ctrl is supposed to be the target setting and it is supposed to
evolve as it progresses.

> 	i.e.
> 	pci_std_enable_acs()
> 	__pci_config_acs(dev, &caps,disable_acs_redir_param,...)
> 	__pci_config_acs(dev, &caps, config_acs_param, 0, 0)
> 
> Here kernel command line takes precedence over 'pci_std_enable_acs()'.
> 'config_acs' takes precedence over 'disable_acs_redir'. IOW, if config_acs
> param is used then it takes the final control over what value is getting
> written to ACS CTRL reg and that is how it should be, no?

Yes, but X in config_acs should copy the FW value not the value
modified by disable_acs_redir_param

Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/8/25 07:10, Jason Gunthorpe wrote:
> On Tue, Jan 07, 2025 at 06:32:42PM -0800, Tushar Dave wrote:
>>
>>
>> On 1/6/25 16:10, Jason Gunthorpe wrote:
>>> On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:
>>>
>>>>>> @@ -1028,10 +1031,10 @@ 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);
>>>>>> -	/* 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;
>>>>>> +	caps->ctrl &= ~mask;
>>>>>> +	caps->ctrl |= (flags & mask);
>>>>>
>>>>> And why delete fw_ctrl? Doesn't that break the unchanged
>>>>> functionality?
>>>>
>>>> No, it does not break the unchanged functionality. I removed it because it
>>>> is not needed after my fix.
>>>
>>> I mean, the whole hunk above is not needed, right? Or at least I don't
>>> see how it relates to your commit message..
>>
>> Without the above hunk, there are cases where ACS flags do not get set
>> correctly. Here is the example test case without above hunk in my patch (IOW
>> keeping fw_ctrl changes as it is like original code plus pci_dbg to print
>> debug info)
> 
> Isn't that because the bit logic in the code is wrong? >
>> -	/* If mask is 0 then we copy the bit from the firmware setting. */
>> -	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
> 
> That comment doesn't match the calculation at all.

Yup, the above bit logic is wrong.

> 
>>>> If it helps, using 'config_acs' the code only allows to configuresIts the lower
>>>> 7 bits of ACS ctrl for the specified PCI device(s).
>>>> The bits other than the lower 7 bits of ACS ctrl remain unchanged.
>>>> The bits specified with 'x' or 'X' that are within the 7 lower bits remain
>>>> unchanged. Trying to configure bits other than lower 7 bits generates an
>>>> error message "Invalid ACS flags specified"
>>>
>>> But the fw_ctrl was how the x behavior was supposed to be implemented,
>>> IIRC there were cases where you could not just rely on caps->ctrl as
>>> something prior had alreaded changed it.
>>
>> I read your comment in https://lore.kernel.org/all/20240508131427.GH4650@nvidia.com/
>>
>> Looking at the current code, the sequence begin with function
>> 'pci_enable_acs()' that reads PCI_ACS_CTRL into caps->ctrl and invoke the
>> below three functions that prepares caps->ctrl before writing to ACS CTRL
>> reg.
> 
> caps->ctrl is supposed to be the target setting and it is supposed to
> evolve as it progresses.

agree.

> 
>> 	i.e.
>> 	pci_std_enable_acs()
>> 	__pci_config_acs(dev, &caps,disable_acs_redir_param,...)
>> 	__pci_config_acs(dev, &caps, config_acs_param, 0, 0)
>>
>> Here kernel command line takes precedence over 'pci_std_enable_acs()'.
>> 'config_acs' takes precedence over 'disable_acs_redir'. IOW, if config_acs
>> param is used then it takes the final control over what value is getting
>> written to ACS CTRL reg and that is how it should be, no?
> 
> Yes, but X in config_acs should copy the FW value not the value
> modified by disable_acs_redir_param

I see your point. In that case (for the last hunk in my patch) something like 
this should work IMO.

-       /* 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 unchanged ACS bits 'x' or 'X', copy the bits from the firmware 
setting. */
+        if (!acs_mask)
+                caps->ctrl = caps->fw_ctrl;
+
+       caps->ctrl &= ~mask;
+       caps->ctrl |= (flags & mask);

Wish I can have better condition check instead of 'if (!acs_mask)' but let me 
know your thoughts.


-Tushar

> 
> Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe]

On Wed, Jan 08, 2025 at 07:13:34PM -0800, Tushar Dave wrote:
> > Yes, but X in config_acs should copy the FW value not the value
> > modified by disable_acs_redir_param
> 
> I see your point. In that case (for the last hunk in my patch) something
> like this should work IMO.
> 
> -       /* 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 unchanged ACS bits 'x' or 'X', copy the bits from the
> firmware setting. */
> +        if (!acs_mask)
> +                caps->ctrl = caps->fw_ctrl;
> +
> +       caps->ctrl &= ~mask;
> +       caps->ctrl |= (flags & mask);
> 
> Wish I can have better condition check instead of 'if (!acs_mask)' but let
> me know your thoughts.

It should be per-bit surely? I think the original logic was the right
idea, just the bit logic had the wrong operators..

Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/13/25 12:07, Jason Gunthorpe wrote:
> On Wed, Jan 08, 2025 at 07:13:34PM -0800, Tushar Dave wrote:
>>> Yes, but X in config_acs should copy the FW value not the value
>>> modified by disable_acs_redir_param
>>
>> I see your point. In that case (for the last hunk in my patch) something
>> like this should work IMO.
>>
>> -       /* 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 unchanged ACS bits 'x' or 'X', copy the bits from the
>> firmware setting. */
>> +        if (!acs_mask)
>> +                caps->ctrl = caps->fw_ctrl;
>> +
>> +       caps->ctrl &= ~mask;
>> +       caps->ctrl |= (flags & mask);
>>
>> Wish I can have better condition check instead of 'if (!acs_mask)' but let
>> me know your thoughts.
> 
> It should be per-bit surely? I think the original logic was the right
> idea, just the bit logic had the wrong operators..

Here is the updated _last_ hunk of my patch with original idea but bit logic 
corrected:


@@ -1028,10 +1032,15 @@ 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;
+       caps->ctrl = (caps->ctrl & mask) | (caps->fw_ctrl & ~mask);
+
+       /* Apply the flags */
+       caps->ctrl &= ~mask;
+       caps->ctrl |= (flags & mask);

         pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
  }



Some test results with debug enabled,

Test 1: 
pci=config_acs=xxx1000@0000:02:00.0;101xxxx@0009:00:00.0;0x1x0x1@0030:02:00.0

[   12.383327] pci 0000:02:00.0: ACS mask  = 0x000f
[   12.383330] pci 0000:02:00.0: ACS flags = 0x0008
[   12.383332] pci 0000:02:00.0: ACS control = 0x005f
[   12.383334] pci 0000:02:00.0: ACS fw_ctrl = 0x005b
[   12.383334] pci 0000:02:00.0: Configured ACS to 0x0058
[   15.416919] pci 0009:00:00.0: ACS mask  = 0x0070
[   15.416920] pci 0009:00:00.0: ACS flags = 0x0050
[   15.416921] pci 0009:00:00.0: ACS control = 0x001d
[   15.416922] pci 0009:00:00.0: ACS fw_ctrl = 0x0000
[   15.416922] pci 0009:00:00.0: Configured ACS to 0x0050
[   22.271312] pci 0030:02:00.0: ACS mask  = 0x0055
[   22.271313] pci 0030:02:00.0: ACS flags = 0x0011
[   22.271314] pci 0030:02:00.0: ACS control = 0x005f
[   22.271315] pci 0030:02:00.0: ACS fw_ctrl = 0x005f
[   22.271316] pci 0030:02:00.0: Configured ACS to 0x001b



Test 2: 
pci=config_acs=11111xx@0000:02:00.0;xxx1000@0009:00:00.0;111xxxx@0030:02:00.0

[   12.430316] pci 0000:02:00.0: ACS mask  = 0x007c
[   12.430317] pci 0000:02:00.0: ACS flags = 0x007c
[   12.430318] pci 0000:02:00.0: ACS control = 0x005d
[   12.430318] pci 0000:02:00.0: ACS fw_ctrl = 0x0058
[   12.430319] pci 0000:02:00.0: Configured ACS to 0x007c
[   15.461740] pci 0009:00:00.0: ACS mask  = 0x000f
[   15.461742] pci 0009:00:00.0: ACS flags = 0x0008
[   15.461743] pci 0009:00:00.0: ACS control = 0x001d
[   15.461745] pci 0009:00:00.0: ACS fw_ctrl = 0x0000
[   15.461746] pci 0009:00:00.0: Configured ACS to 0x0008
[   22.362102] pci 0030:02:00.0: ACS mask  = 0x0070
[   22.362104] pci 0030:02:00.0: ACS flags = 0x0070
[   22.362105] pci 0030:02:00.0: ACS control = 0x001f
[   22.362106] pci 0030:02:00.0: ACS fw_ctrl = 0x001b
[   22.362107] pci 0030:02:00.0: Configured ACS to 0x007b



Test 3: pci=disable_acs_redir=0000:02:00.0;0009:00:00.0;0030:02:00.0

[   12.425584] pci 0000:02:00.0: ACS mask  = 0x002c
[   12.425585] pci 0000:02:00.0: ACS flags = 0xffd3
[   12.425586] pci 0000:02:00.0: ACS control = 0x007d
[   12.425587] pci 0000:02:00.0: ACS fw_ctrl = 0x007c
[   12.425588] pci 0000:02:00.0: Configured ACS to 0x0050
[   15.460079] pci 0009:00:00.0: ACS mask  = 0x002c
[   15.460081] pci 0009:00:00.0: ACS flags = 0xffd3
[   15.460082] pci 0009:00:00.0: ACS control = 0x001d
[   15.460083] pci 0009:00:00.0: ACS fw_ctrl = 0x0000
[   15.460084] pci 0009:00:00.0: Configured ACS to 0x0000
[   22.347454] pci 0030:02:00.0: ACS mask  = 0x002c
[   22.347455] pci 0030:02:00.0: ACS flags = 0xffd3
[   22.347458] pci 0030:02:00.0: ACS control = 0x007f
[   22.347459] pci 0030:02:00.0: ACS fw_ctrl = 0x007b
[   22.347462] pci 0030:02:00.0: Configured ACS to 0x0053



-Tushar

> 
> Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe]

On Wed, Jan 15, 2025 at 07:11:25PM -0800, Tushar Dave wrote:
> @@ -1028,10 +1032,15 @@ 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;
> +       caps->ctrl = (caps->ctrl & mask) | (caps->fw_ctrl & ~mask);
> +
> +       /* Apply the flags */
> +       caps->ctrl &= ~mask;
> +       caps->ctrl |= (flags & mask);

caps->ctrl = (caps->ctrl & mask) & ~mask == 0 - so this is kind of confusing.

What we want is to take the fw_ctl for all bits where mask is 0 and
take flags for all bits where it is 1?

  caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);

?

Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/16/25 11:01, Jason Gunthorpe wrote:
> On Wed, Jan 15, 2025 at 07:11:25PM -0800, Tushar Dave wrote:
>> @@ -1028,10 +1032,15 @@ 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;
>> +       caps->ctrl = (caps->ctrl & mask) | (caps->fw_ctrl & ~mask);
>> +
>> +       /* Apply the flags */
>> +       caps->ctrl &= ~mask;
>> +       caps->ctrl |= (flags & mask);
> 
> caps->ctrl = (caps->ctrl & mask) & ~mask == 0 - so this is kind of confusing.
> 
> What we want is to take the fw_ctl for all bits where mask is 0 and
> take flags for all bits where it is 1?
> 
>    caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
> 
> ?

Yes :)

-       /* 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);


I ran some sanity tests and looks good. Can I send V2 now?

-Tushar

> 
> Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe]

On Thu, Jan 16, 2025 at 05:21:29PM -0800, Tushar Dave wrote:

> -       /* 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);
> 
> 
> I ran some sanity tests and looks good. Can I send V2 now?

I think so, I'm just wondering if this is not quite it either - this
will always throw away any changes any of the other calls made to
ctrl prior to getting here.

So we skip the above if the kernel command line does not refer to the
device?

Ie if the command line refers to the device then it always superceeds
everything, otherwise the other stuff keeps working the way it worked
before??

Jason

Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Tushar Dave]

On 1/17/25 05:28, Jason Gunthorpe wrote:
> On Thu, Jan 16, 2025 at 05:21:29PM -0800, Tushar Dave wrote:
> 
>> -       /* 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);
>>
>>
>> I ran some sanity tests and looks good. Can I send V2 now?
> 
> I think so, I'm just wondering if this is not quite it either - this
> will always throw away any changes any of the other calls made to
> ctrl prior to getting here.

Yes.

> 
> So we skip the above if the kernel command line does not refer to the
> device?

That is correct as well.

> 
> Ie if the command line refers to the device then it always superceeds
> everything, otherwise the other stuff keeps working the way it worked
> before??

Yup.
If kernel commandline is present (either config_acs or disable_acs_redir) and 
device match occurs in function '__pci_config_acs', we discard any changes done 
to ctrl by kernel prior to this call. Else we do not touch ctrl and it works as 
before.

And IMO, that is how it should be. This gives admin a control to configure ACS 
using kernel parameter based on ACS value that FW has set (and not kernel).

-Tushar

>
> Jason