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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6ea9260b-f9cd-4128-b424-11afe6579fdc@nvidia.com>
Date: Wed, 15 Jan 2025 19:11:25 -0800
From: Tushar Dave <tdave@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: corbet@....net, bhelgaas@...gle.com, paulmck@...nel.org,
 akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
 xiongwei.song@...driver.com, vidyas@...dia.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, vsethi@...dia.com,
 sdonthineni@...dia.com
Subject: Re: [PATCH 1/1] PCI: Fix Extend ACS configurability



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@...0:02:00.0;101xxxx@...9:00:00.0;0x1x0x1@...0: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@...0:02:00.0;xxx1000@...9:00:00.0;111xxxx@...0: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