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
