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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c9aeb7a0-5fef-49a5-9ebb-c0e7f3b0fd3e@nvidia.com>
Date: Tue, 7 Jan 2025 18:32:42 -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/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)


Kernel command line: pci=config_acs=xxx1010@pci:15b3:1979;1111@pci:10de:22b1

	pci 0000:02:00.0: ACS mask  = 0x000f
	pci 0000:02:00.0: ACS flags = 0x000a
	pci 0000:02:00.0: ACS control = 0x007f
	pci 0000:02:00.0: ACS fw_ctrl = 0x007f
	pci 0000:02:00.0: Configured ACS to 0x007f
	...
	..
	.
	pci 0039:00:00.0: ACS mask  = 0x000f
	pci 0039:00:00.0: ACS flags = 0x000f
	pci 0039:00:00.0: ACS control = 0x001d
	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
	pci 0039:00:00.0: Configured ACS to 0x001f

As seen in the above output, ACS bits for BDF 0000:02:00.0 did not get set as 
expected. ACS ctrl for BDF 0000:02:00.0 should be 0x7a and not 0x7f.

> 
>> 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"
> 
> 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.

	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?

> 
> What about your fix to the mask changes this reasoning? If nothing
> then it should be its own patch with its own explanation..

Sure. As soon as we are align I'll take care of it.

> 
> Jason