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

[Tushar Dave]

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