Re: [PATCH v4 1/2] PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Sat,  7 Feb 2026 19:30:58 +0800
Wei Wang <wei.w.wang@hotmail.com> wrote:

> The ACS Enhanced Capability introduces several new access controls to
> improve device isolation. These new controls are particularly important
> for device passthrough in virtualization scenarios.
> 
> For example, a DMA transaction from a device may target a guest physical
> address that lies within the memory aperture of the switch's upstream
> port, but not within any memory aperture or BAR space of a downstream
> port. In such cases, the switch would generate an Unsupported Request (UR)
> response to the device, which is undesirable. Enabling Unclaimed Request
> Redirect Control ensures that these DMA requests are forwarded upstream
> instead of being rejected.
> 
> The ACS DSP and USP Memory Target Access Control and ACS I/O Request
> Blocking features similarly enhance device isolation. Device grouping in
> Linux assumes that devices are properly isolated. Therefore, enable these
> controls by default if PCI_ACS_ECAP is supported by the hardware. As with
> other basic ACS access controls, these new controls can be configured via
> the "config_acs = " boot parameter.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Hi Wei Wang,

I'm late to the game here and may be missing something but
I think the validation and defines for the 2 bit fields are both
misleading and not sufficiently careful to reject values that
are not allowed.  Maybe we do want to allow the reserved value
but if so use a 2 bit mask to validate, not two specific
values that the filed can take.

Thanks,

Jonathan

> ---
>  .../admin-guide/kernel-parameters.txt         | 23 +++++++++++++------
>  drivers/pci/pci.c                             | 13 ++++++++++-
>  include/uapi/linux/pci_regs.h                 |  7 ++++++
>  3 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7cfbf1102152..fd895716b07d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5301,13 +5301,22 @@ Kernel parameters
>  				flags.
>  
>  				ACS Flags is defined as follows:
> -				  bit-0 : ACS Source Validation
> -				  bit-1 : ACS Translation Blocking
> -				  bit-2 : ACS P2P Request Redirect
> -				  bit-3 : ACS P2P Completion Redirect
> -				  bit-4 : ACS Upstream Forwarding
> -				  bit-5 : ACS P2P Egress Control
> -				  bit-6 : ACS Direct Translated P2P
> +				  bit-0     : ACS Source Validation
> +				  bit-1     : ACS Translation Blocking
> +				  bit-2     : ACS P2P Request Redirect
> +				  bit-3     : ACS P2P Completion Redirect
> +				  bit-4     : ACS Upstream Forwarding
> +				  bit-5     : ACS P2P Egress Control
> +				  bit-6     : ACS Direct Translated P2P
> +				  bit-7     : ACS I/O Request Blocking
> +				  bit-9:8   : ACS DSP Memory Target Access Ctrl
> +				      00    : Direct Request access enabled
> +				      01    : Request blocking enabled
> +				      10    : Request redirect enabled
> +				      11    : Reserved
> +				  bit-11:10 : ACS USP Memory Target Access Ctrl
> +				              Same encoding as bit-9:8
> +				  bit-12    : ACS Unclaimed Request Redirect Ctrl
>  				Each bit can be marked as:
>  				  '0' – force disabled
>  				  '1' – force enabled
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d2810eb97bda..1714e29ce099 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -942,7 +942,10 @@ 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_ACS_UF | PCI_ACS_EC | PCI_ACS_DT | PCI_ACS_IB |
> +			    PCI_ACS_DMAC_RB | PCI_ACS_DMAC_RR |
> +			    PCI_ACS_UMAC_RB | PCI_ACS_UMAC_RR |
> +			    PCI_ACS_URRC)) {
This used of the RB and RR Bits seems misleading to me as they form parts
of 2 bit field that can only take values 0, 1 and 2.

So if the command line set PCI_ACS_UMAC_RB and PCI_ACS_UMAC_RR it
would have hit the reserved value and that should be detected.

Hence I think you need to do this sort of masking + additional checks that these
2 bit fields have valid content.

>  			pci_err(dev, "Invalid ACS flags specified\n");
>  			return;
>  		}
> @@ -1002,6 +1005,14 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>  	/* Upstream Forwarding */
>  	caps->ctrl |= (caps->cap & PCI_ACS_UF);
>  
> +	/*
> +	 * Downstream and Upstream Port Memory Target Access Redirect,
> +	 * Redirect Unclaimed Request Redirect Control
> +	 */
> +	if (caps->cap & PCI_ACS_ECAP)
> +		caps->ctrl |= PCI_ACS_DMAC_RR | PCI_ACS_UMAC_RR |
> +			      PCI_ACS_URRC | PCI_ACS_IB;
> +
>  	/* Enable Translation Blocking for external devices and noats */
>  	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
>  		caps->ctrl |= (caps->cap & PCI_ACS_TB);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index ec1c54b5a310..593ef522d2ba 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1016,6 +1016,7 @@
>  
>  /* Access Control Service */
>  #define PCI_ACS_CAP		0x04	/* ACS Capability Register */
> +#define  PCI_ACS_ECAP		0x0080  /* ACS Enhanced Capability */
>  #define  PCI_ACS_SV		0x0001	/* Source Validation */
>  #define  PCI_ACS_TB		0x0002	/* Translation Blocking */
>  #define  PCI_ACS_RR		0x0004	/* P2P Request Redirect */
> @@ -1023,6 +1024,12 @@
>  #define  PCI_ACS_UF		0x0010	/* Upstream Forwarding */
>  #define  PCI_ACS_EC		0x0020	/* P2P Egress Control */
>  #define  PCI_ACS_DT		0x0040	/* Direct Translated P2P */
> +#define  PCI_ACS_IB		0x0080	/* I/O Request Blocking */
> +#define  PCI_ACS_DMAC_RB	0x0100  /* DSP Memory Target Access Blocking */
> +#define  PCI_ACS_DMAC_RR	0x0200  /* DSP Memory Target Access Redirect */
> +#define  PCI_ACS_UMAC_RB	0x0400  /* USP Memory Target Access Blocking */

If I'm reading this write, those are specific values in the
ACS USP Memory Target Access Control, not what they appear to be here
which is bits that can be combined.
I think you need a _MASK and separate definitions for the 3 values that
are allowed.

> +#define  PCI_ACS_UMAC_RR	0x0800  /* USP Memory Target Access Redirect */
> +#define  PCI_ACS_URRC		0x1000	/* Unclaimed Request Redirect Ctrl */
>  #define PCI_ACS_EGRESS_BITS	0x05	/* ACS Egress Control Vector Size */
>  #define PCI_ACS_CTRL		0x06	/* ACS Control Register */
>  #define PCI_ACS_EGRESS_CTL_V	0x08	/* ACS Egress Control Vector */