Re: [PATCH v3 09/11] PCI: Enable ACS Enhanced bits for enable_acs and config_acs

[Donald Dutile <ddutile@redhat.com>]

On 9/5/25 2:06 PM, Jason Gunthorpe wrote:
> The ACS Enhanced bits are intended to address a lack of precision in the
> spec about what ACS P2P Request Redirect is supposed to do. While Linux
> has long assumed that PCI_ACS_RR would cover MMIO BARs located in the root
> port and PCIe Switch ports, the spec took the position that it is
> implementation specific.
> 
> To get the behavior Linux has long assumed it should be setting:
> 
>    PCI_ACS_RR | PCI_ACS_DSP_MT_RR | PCI_ACS_USP_MT_RR | PCI_ACS_UNCLAMED_RR
> 
> Follow this guidance in enable_acs and set the additional bits if ACS
> Enhanced is supported.
> 
> Allow config_acs to control these bits if the device has ACS Enhanced.
> 
> The spec permits the HW to wire the bits, so after setting them
> pci_acs_flags_enabled() does do a pci_read_config_word() to read the
> actual value in effect.
> 
> Note that currently Linux sets these bits to 0, so any new HW that comes
> supporting ACS Enhanced will end up with historical Linux disabling these
> functions. Devices wanting to be compatible with old Linux will need to
> wire the ctrl bits to follow ACS_RR. Devices that implement ACS Enhanced
> and support the ctrl=0 behavior will break PASID SVA support and VFIO
> isolation when ACS_RR is enabled.
> 
> Due to the above I strongly encourage backporting this change otherwise
> old kernels may have issues with new generations of PCI switches.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/pci/pci.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0f4d98036cddd..983f71211f0055 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -957,6 +957,7 @@ 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 flags = acs_flags;
> +	u16 supported_flags;
>   	u16 mask = acs_mask;
>   	char *delimit;
>   	int ret = 0;
> @@ -1001,8 +1002,14 @@ 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)) {
> +		supported_flags = PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> +				  PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_EC |
> +				  PCI_ACS_DT;
> +		if (caps->cap & PCI_ACS_ENHANCED)
> +			supported_flags |= PCI_ACS_USP_MT_RR |
> +					   PCI_ACS_DSP_MT_RR |
> +					   PCI_ACS_UNCLAIMED_RR;
> +		if (mask & ~supported_flags) {
>   			pci_err(dev, "Invalid ACS flags specified\n");
>   			return;
>   		}
> @@ -1062,6 +1069,14 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>   	/* Upstream Forwarding */
>   	caps->ctrl |= (caps->cap & PCI_ACS_UF);
>   
> +	/*
> +	 * USP/DSP Memory Target Access Control and Unclaimed Request Redirect
> +	 */
> +	if (caps->cap & PCI_ACS_ENHANCED) {
> +		caps->ctrl |= PCI_ACS_USP_MT_RR | PCI_ACS_DSP_MT_RR |
> +			      PCI_ACS_UNCLAIMED_RR;
> +	}
> +
>   	/* Enable Translation Blocking for external devices and noats */
>   	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
>   		caps->ctrl |= (caps->cap & PCI_ACS_TB);

Reviewed-by: Donald Dutile <ddutile@redhat.com>