Re: [PATCH v4 11/18] PCI/P2PDMA: Derive peer-to-peer routing from ACS control bits

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ACS Egress Control (EC) is incorrectly bypassed for Completion TLPs, violating the PCIe specification and allowing unworkable P2P DMA paths to be mapped as direct.
--

--- Patch [11]: [PATCH v4 11/18] PCI/P2PDMA: Derive peer-to-peer routing from ACS control bits ---
commit e4c8e3c20c328ff7022188129cc2873b1613420b
Author: Leon Romanovsky <[email protected]>

PCI/P2PDMA: Derive peer-to-peer routing from ACS control bits

This commit modifies the P2PDMA ACS redirection check to separate the logic
for Request and Completion TLPs. It evaluates Request Redirect for client
requests and Completion Redirect for provider read completions.

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index a364008bbf504..1eda7b2135d70 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c

[ ... ]

> @@ -536,26 +538,35 @@ static struct pci_dev *find_parent_pci_dev(struct device *dev)
>  	return NULL;
>  }
>  
> -/*
> - * Check if a PCI bridge has its ACS redirection bits set to redirect P2P
> - * TLPs upstream via ACS. Returns 1 if the packets will be redirected
> - * upstream, 0 otherwise.
> - */
> -static int pci_bridge_has_acs_redir(struct pci_dev *pdev)
> +enum pci_acs_p2pdma_state {
> +	PCI_ACS_P2PDMA_DIRECT,
> +	PCI_ACS_P2PDMA_REDIRECT,
> +};
> +
> +enum pci_acs_p2pdma_tlp {
> +	PCI_ACS_P2PDMA_TLP_REQUEST,
> +	PCI_ACS_P2PDMA_TLP_COMPLETION,
> +};
> +
> +static enum pci_acs_p2pdma_state
> +pci_acs_p2pdma_state(struct pci_dev *pdev, enum pci_acs_p2pdma_tlp tlp)
>  {
>  	int pos;
>  	u16 ctrl;
>  
>  	pos = pdev->acs_cap;
>  	if (!pos)
> -		return 0;
> +		return PCI_ACS_P2PDMA_DIRECT;
>  
> -	pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
> +	if (pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl))
> +		return PCI_ACS_P2PDMA_REDIRECT;
>  
> -	if (ctrl & (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC))
> -		return 1;
> +	if (tlp == PCI_ACS_P2PDMA_TLP_COMPLETION)
> +		return ctrl & PCI_ACS_CR ? PCI_ACS_P2PDMA_REDIRECT :
> +					   PCI_ACS_P2PDMA_DIRECT;

[Severity: High]
According to PCIe Base Specification 6.12.4, ACS Egress Control applies to
all TLPs, including both Requests and Completions.

Does this logic incorrectly bypass Egress Control (PCI_ACS_EC) for Completion
TLPs by only evaluating Completion Redirect (PCI_ACS_CR)?

If a PCIe switch has ACS Egress Control configured to isolate P2P traffic,
will the hardware drop P2P Read Completions as ACS violations while the
kernel incorrectly maps the path as direct? This could lead to Completion
Timeouts and potential runtime failures when drivers attempt P2P DMA reads.

> -	return 0;
> +	return ctrl & (PCI_ACS_RR | PCI_ACS_EC) ?
> +		PCI_ACS_P2PDMA_REDIRECT : PCI_ACS_P2PDMA_DIRECT;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11