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

Vidya Sagar
2024-12-14T12:31:00Z

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