Re: [PATCH v4 2/2] PCI: Add the enhanced ACS controls check to pci_acs_flags_enabled()

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

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

> The enhanced ACS controls introduced by PCIe Gen 5 ensures better device
> isolation. On devices that support the PCI_ACS_ECAP capability, the
> controls are required to be enabled properly:
> - ACS I/O Request Blocking needs to be enabled to avoid unintended
>   upstream I/O requests.
> - ACS DSP and USP Memory Target Access Control needs to be set with
>   Request Redirect or Request Blocking to ensure the Downstream and
>   Upstream Port memory resource ranges are not accessed by upstream
>   memory requests.
> - ACS Unclaimed Request Redirect needs to be enabled to ensure accesses to
>   areas that lies within a Switch's Upstream Port memory apertures but not
>   within any Downstream Port memory apertures get redirected.
> 
> To maintain compatibility with legacy devices that lack PCI_ACS_ECAP
> support, pci_acs_enabled() skips checking for the capability.
> 
> Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
Hi Wei Wang,

A few things inline.

Thanks,

Jonathan

> ---
>  drivers/pci/pci.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1714e29ce099..53e79948b4ea 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

> +static bool pci_acs_ecap_enabled(struct pci_dev *pdev, u16 ctrl)
> +{
> +	bool is_dsp = pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM;
> +	struct pci_dev *usp_pdev = pci_upstream_bridge(pdev);
> +	u16 mask = PCI_ACS_DMAC_RB | PCI_ACS_DMAC_RR;
> +
> +	/*
> +	 * For ACS DSP/USP Memory Target Access Control, either Request
> +	 * Redirect or Request Blocking must be enabled to enforce isolation.
> +	 * According to PCIe spec 7.0, the DSP Memory Target Access is
> +	 * applicable to both Root Ports and Switch Upstream Ports that have
> +	 * applicable Memory BAR space to protect. So if the device does not
> +	 * have a Memory BAR, it skips the check.
> +	 */
> +	if (pci_dev_has_memory_bars(pdev) &&
> +	    (ctrl & mask) != PCI_ACS_DMAC_RB &&
> +	    (ctrl & mask) != PCI_ACS_DMAC_RR)

As below. I'd use the mask define suggested in previous then FIELD_GET()
plus checking the value of that against defines for these two field
values.

> +		return false;
> +
> +	mask = PCI_ACS_UMAC_RB | PCI_ACS_UMAC_RR;
This is the mask for the field that should be in the header.
> +	/*
> +	 * The USP Memory Target Access is only applicable to downstream ports
> +	 * that have applicable Memory BAR space in the Switch Upstream Port to
> +	 * protect.
> +	 */
> +	if (is_dsp && pci_dev_has_memory_bars(usp_pdev) &&
> +	    (ctrl & mask) != PCI_ACS_UMAC_RB &&
> +	    (ctrl & mask) != PCI_ACS_UMAC_RR)
> +		return false;
> +
> +	/* PCI_ACS_URRC is applicable to Downstream Ports only.  */
> +	if (is_dsp && !(ctrl & PCI_ACS_URRC))
> +		return false;

I'd be tempted to group the DSP specific handling and drop the local variable.

	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
		if (pci_dev_has_memory_bars(usp_pdev) &&
		    (ctrl & mask) != PCI_ACS_UMAC_RB &&
		    (ctrl & mask) != PCI_ACS_UMAC_RR)
// or use FIELD_GET() to get using the mask suggested in previous patch then
// match what was in the filed here.
			return false;
	
		if (!(ctrl & PCI_ACS_URRC))
			return false;
	}
> +
> +	/* PCI_ACS_IB is applicable to both Root and Downstream Ports. */
> +	return !!(ctrl & PCI_ACS_IB);
> +}
> +