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

[Tushar Dave <tdave@nvidia.com>]

On 1/8/25 07:10, Jason Gunthorpe wrote:
> On Tue, Jan 07, 2025 at 06:32:42PM -0800, Tushar Dave wrote:
>>
>>
>> On 1/6/25 16:10, Jason Gunthorpe wrote:
>>> On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote:
>>>
>>>>>> @@ -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.
>>>
>>> I mean, the whole hunk above is not needed, right? Or at least I don't
>>> see how it relates to your commit message..
>>
>> Without the above hunk, there are cases where ACS flags do not get set
>> correctly. Here is the example test case without above hunk in my patch (IOW
>> keeping fw_ctrl changes as it is like original code plus pci_dbg to print
>> debug info)
> 
> Isn't that because the bit logic in the code is wrong? >
>> -	/* If mask is 0 then we copy the bit from the firmware setting. */
>> -	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
> 
> That comment doesn't match the calculation at all.

Yup, the above bit logic is wrong.

> 
>>>> If it helps, using 'config_acs' the code only allows to configuresIts 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"
>>>
>>> But the fw_ctrl was how the x behavior was supposed to be implemented,
>>> IIRC there were cases where you could not just rely on caps->ctrl as
>>> something prior had alreaded changed it.
>>
>> I read your comment in https://lore.kernel.org/all/20240508131427.GH4650@nvidia.com/
>>
>> Looking at the current code, the sequence begin with function
>> 'pci_enable_acs()' that reads PCI_ACS_CTRL into caps->ctrl and invoke the
>> below three functions that prepares caps->ctrl before writing to ACS CTRL
>> reg.
> 
> caps->ctrl is supposed to be the target setting and it is supposed to
> evolve as it progresses.

agree.

> 
>> 	i.e.
>> 	pci_std_enable_acs()
>> 	__pci_config_acs(dev, &caps,disable_acs_redir_param,...)
>> 	__pci_config_acs(dev, &caps, config_acs_param, 0, 0)
>>
>> Here kernel command line takes precedence over 'pci_std_enable_acs()'.
>> 'config_acs' takes precedence over 'disable_acs_redir'. IOW, if config_acs
>> param is used then it takes the final control over what value is getting
>> written to ACS CTRL reg and that is how it should be, no?
> 
> 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.


-Tushar

> 
> Jason