* [RESEND PATCH v2] PCI: Fix Extend ACS configurability
@ 2025-02-07 3:03 Tushar Dave
2025-02-07 14:34 ` Jason Gunthorpe
2025-02-21 22:23 ` Bjorn Helgaas
0 siblings, 2 replies; 4+ messages in thread
From: Tushar Dave @ 2025-02-07 3:03 UTC (permalink / raw)
To: jgg, corbet, bhelgaas, paulmck, akpm, thuth, rostedt,
xiongwei.song, vidyas, linux-doc, linux-kernel, linux-pci
Cc: vsethi, sdonthineni, Tushar Dave
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(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..c1ab5d50112d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -954,8 +954,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;
@@ -963,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) {
@@ -971,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') {
@@ -1027,10 +1031,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);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH v2] PCI: Fix Extend ACS configurability
2025-02-07 3:03 [RESEND PATCH v2] PCI: Fix Extend ACS configurability Tushar Dave
@ 2025-02-07 14:34 ` Jason Gunthorpe
2025-02-20 0:31 ` Tushar Dave
2025-02-21 22:23 ` Bjorn Helgaas
1 sibling, 1 reply; 4+ messages in thread
From: Jason Gunthorpe @ 2025-02-07 14:34 UTC (permalink / raw)
To: Tushar Dave
Cc: corbet, bhelgaas, paulmck, akpm, thuth, rostedt, xiongwei.song,
vidyas, linux-doc, linux-kernel, linux-pci, vsethi, sdonthineni
On Thu, Feb 06, 2025 at 07:03:38PM -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.
>
> 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.
..
> 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(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH v2] PCI: Fix Extend ACS configurability
2025-02-07 14:34 ` Jason Gunthorpe
@ 2025-02-20 0:31 ` Tushar Dave
0 siblings, 0 replies; 4+ messages in thread
From: Tushar Dave @ 2025-02-20 0:31 UTC (permalink / raw)
To: Jason Gunthorpe, bhelgaas
Cc: corbet, paulmck, akpm, thuth, rostedt, xiongwei.song, vidyas,
linux-doc, linux-kernel, linux-pci, vsethi, sdonthineni
On 2/7/25 06:34, Jason Gunthorpe wrote:
> On Thu, Feb 06, 2025 at 07:03:38PM -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.
>>
>> 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.
> ..
>
>> 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(-)
>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>
> Jason
Hi Bjorn,
Gentle reminder. Let me know if there are any review comments!
Thanks.
-Tushar
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH v2] PCI: Fix Extend ACS configurability
2025-02-07 3:03 [RESEND PATCH v2] PCI: Fix Extend ACS configurability Tushar Dave
2025-02-07 14:34 ` Jason Gunthorpe
@ 2025-02-21 22:23 ` Bjorn Helgaas
1 sibling, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2025-02-21 22:23 UTC (permalink / raw)
To: Tushar Dave
Cc: jgg, corbet, bhelgaas, paulmck, akpm, thuth, rostedt,
xiongwei.song, vidyas, linux-doc, linux-kernel, linux-pci, vsethi,
sdonthineni
On Thu, Feb 06, 2025 at 07:03:38PM -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.
>
> 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>
Applied to pci/acs for v6.15, thanks!
> ---
>
> 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 869d204a70a3..c1ab5d50112d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -954,8 +954,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;
>
> @@ -963,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) {
> @@ -971,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') {
> @@ -1027,10 +1031,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);
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-02-21 22:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-07 3:03 [RESEND PATCH v2] PCI: Fix Extend ACS configurability Tushar Dave
2025-02-07 14:34 ` Jason Gunthorpe
2025-02-20 0:31 ` Tushar Dave
2025-02-21 22:23 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).