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: <20250108151021.GS5556@nvidia.com>
Date: Wed, 8 Jan 2025 11:10:21 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Tushar Dave <tdave@...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 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.

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

caps->ctrl is supposed to be the target setting and it is supposed to
evolve as it progresses.

> 	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

Jason