Re: [PATCH v9 1/6] PCI: Validate ACS control bits against device-specific ACS capabilities

[Ilpo Järvinen]

On Thu, 3 Sep 2026, Wei Wang wrote:

> The ACS control validation used the kernel's full set of ACS control bits,
> which allowed users to request ACS features that the device does not
> support. Because hardware silently ignores these unsupported bits, users
> have no indication that their config_acs= request was partially
> ineffective.
> 
> Validate the requested ACS control bits against dev->acs_capabilities so
> that only device-supported ACS controls are accepted. Mask the capability
> with GENMASK_U16(6, 0) to select only the currently defined ACS control
> bits (0-6). Higher bits in the capability structure (e.g. the egress
> control vector size in bits 8-15) do not correspond to control bits in
> the ACS control register. Add a debug message to report which unsupported
> bits were ignored.
> 
> __pci_config_acs() is also called from disable_acs_redir_param(), which
> passes a hardcoded acs_mask (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC) that
> does not need validation. Gate the new check on !acs_mask so it only
> fires for the config_acs_param path, which always invokes
> __pci_config_acs() with acs_mask == 0 and builds the actual mask from
> user input.
> 
> Also move the check after the device is matched, since the ctrl bits apply
> only to the matched device.
> 
> Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxxxx>
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
>  drivers/pci/pci.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f8..2af679111a9b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -910,6 +910,7 @@ struct pci_acs {
>  static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  			     const char *p, const u16 acs_mask, const u16 acs_flags)
>  {
> +	u16 valid_ctrl = dev->acs_capabilities & GENMASK_U16(6, 0);

This looks a step backwards.

These bits are surely named so this mask should be a composite of those 
define names, either done here or through another define (likely the 
latter is better).

>  	u16 flags = acs_flags;
>  	u16 mask = acs_mask;
>  	char *delimit;
> @@ -955,12 +956,6 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  			}
>  		}
>  
> -		if (mask & ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> -			    PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)) {
> -			pci_err(dev, "Invalid ACS flags specified\n");
> -			return;
> -		}
> -
>  		ret = pci_dev_str_match(dev, p, &p);
>  		if (ret < 0) {
>  			pr_warn_once("PCI: Can't parse ACS command line parameter\n");
> @@ -983,6 +978,13 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  	if (!pci_dev_specific_disable_acs_redir(dev))
>  		return;
>  
> +	if (!acs_mask && (mask & ~valid_ctrl)) {
> +		pci_dbg(dev, "Ignoring unsupported ACS bits: %#06x\n",
> +			mask & ~valid_ctrl);
> +		mask &= valid_ctrl;
> +		flags &= valid_ctrl;
> +	}
> +
>  	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);
> 

-- 
 i.