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

[Tushar Dave]

On 1/2/25 10:40, Jason Gunthorpe wrote:
> On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
>
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index dc663c0ca670..fc1c37910d1c 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -4654,11 +4654,10 @@
> >   				Format:
> >   				<ACS flags>@<pci_dev>[; ...]
> >   				Specify one or more PCI devices (in the format
> > -				specified above) optionally prepended with flags
> > -				and separated by semicolons. The respective
> > -				capabilities will be enabled, disabled or
> > -				unchanged based on what is specified in
> > -				flags.
> > +				specified above) prepended with flags and separated
> > +				by semicolons. The respective capabilities will be
> > +				enabled, disabled or unchanged based on what is
> > +				specified in flags.
> >   
> >   				ACS Flags is defined as follows:
> >   				  bit-0 : ACS Source Validation
> > @@ -4673,7 +4672,7 @@
> >   				  '1' – force enabled
> >   				  'x' – unchanged
> >   				For example,
> > -				  pci=config_acs=10x
> > +				  pci=config_acs=10x@pci:0:0
> >   				would configure all devices that support
> >   				ACS to enable P2P Request Redirect, disable
> >   				Translation Blocking, and leave Source
>
> Is this an unrelated change? The format of the command line shouldn't
> be changed to fix the described bug, why is the documentation changed?

The documentation as it is (i.e. without my patch), is not correct.

IOW, config_acs parameter does require flags and it is not optional. Without 
flags it results into "ACS Flags missing". Therefore I remove word "optionally" 
from the documentation text.

Secondly, the syntax in the example 'pci=config_acs=10x' is incorrect. The 
correct syntax should be 'pci=config_acs=10x@pci:0:0' that would configure all 
devices that support ACS to enable P2P Request Redirect, disable Translation 
Blocking, and leave Source Validation unchanged from whatever power-up or 
firmware set it to.

> >   static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> > -			     const char *p, u16 mask, u16 flags)
> > +			     const char *p, const u16 acs_mask, const u16 acs_flags)
> >   {
> > +	u16 flags = acs_flags;
> > +	u16 mask = acs_mask;
> >   	char *delimit;
> >   	int ret = 0;
> >   
> > @@ -964,7 +965,7 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> >   		return;
> >   
> >   	while (*p) {
> > -		if (!mask) {
> > +		if (!acs_mask) {
> >   			/* Check for ACS flags */
> >   			delimit = strstr(p, "@");
> >   			if (delimit) {
> > @@ -972,6 +973,8 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> >   				u32 shift = 0;
> >   
> >   				end = delimit - p - 1;
> > +				mask = 0;
> > +				flags = 0;
> >   
> >   				while (end > -1) {
> >   					if (*(p + end) == '0') {
>
> This function the entire fix, right? Because the routine was
> clobbering acs_mask as it processed the earlier devices?

yes, that is correct.

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

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"

-Tushar
> Jason