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

Tushar Dave
2025-01-16T03:11:34Z

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