Re: [PATCH v2 08/13] PCI/P2PDMA: Honor ACS egress control vectors

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Cross-domain PCIe ACS Egress Control Vector evaluation uses meaningless port numbers when devices reside on different host bridges.
--

--- Patch [8]: [PATCH v2 08/13] PCI/P2PDMA: Honor ACS egress control vectors ---
commit 96de4ef43ce98491771577d7338b7224cb26b816
Author: Leon Romanovsky <[email protected]>

PCI/P2PDMA: Honor ACS egress control vectors

This commit updates P2PDMA to properly evaluate the ACS Egress Control Vector
when determining routing between peers. It reads the vector where the paths
diverge below their common upstream port, treating a clear vector bit as a
direct path and a set bit (without Request Redirect) as an ACS Violation.

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c

[ ... ]

> @@ -770,25 +770,44 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  	 */
>  	no_common_upstream = true;
>  
> -check_b_path_acs:
> -	bb = b;
> +check_paths_acs:
> +	*dist = dist_a + dist_b;
> +	bb = provider;
>  
>  	while (bb) {
> +		target = bb == a_child ? b_child : NULL;
> +		state = pci_acs_p2pdma_state(bb, target);

[Severity: High]
When no_common_upstream is true, the provider and client reside on different
host bridges. Does this cause pci_acs_p2pdma_state() to evaluate the ACS
Egress Control Vector using a target from a completely different PCIe domain?

If bb == a_child, b_child is passed as the target. Down in
pci_acs_egress_ctrl_set(), the link capabilities of the foreign port are used
to extract a target port number:

drivers/pci/pci.c:pci_acs_egress_ctrl_set() {
    ...
    target_port = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
    ...
    ret = pci_read_config_dword(pdev,
                                pdev->acs_cap + PCI_ACS_EGRESS_CTL_V +
                                (target_port / 32) * sizeof(vector),
                                &vector);
    ...
}

Since pdev and target are on different host bridges, does this use a
meaningless port number as an index? If so, this could read arbitrary bits
from the hardware configuration space and potentially result in incorrect ACS
routing decisions or spurious violation warnings.

> +		if (state != PCI_ACS_P2PDMA_DIRECT) {
> +			seq_buf_print_bus_devfn(&acs_list, bb);
> +			if (state == PCI_ACS_P2PDMA_REDIRECT)
> +				acs_redirect_cnt++;
> +			else if (!acs_unsupported)
> +				acs_unsupported = bb;
> +		}
> +
>  		if (a == bb)
>  			break;
>  
> -		if (pci_acs_p2pdma_state(bb, NULL) ==
> -		    PCI_ACS_P2PDMA_REDIRECT) {
> -			seq_buf_print_bus_devfn(&acs_list, bb);
> -			acs_cnt++;
> -		}
> -
>  		bb = pci_upstream_bridge(bb);
>  	}
>  
> -	*dist = dist_a + dist_b;
> -
> -	if (!acs_cnt) {
> +	bb = client;
> +
> +	while (bb && a != bb) {
> +		target = bb == b_child ? a_child : NULL;
> +		state = pci_acs_p2pdma_state(bb, target);

[Severity: High]
Similarly here, if the devices are on different host bridges, a_child and
b_child are on different domains. Does passing a_child as the target to
b_child's ACS evaluation create the same cross-domain hardware configuration
read issue?

> +		if (state != PCI_ACS_P2PDMA_DIRECT) {
>  			seq_buf_print_bus_devfn(&acs_list, bb);
> -			acs_cnt++;
> +			if (state == PCI_ACS_P2PDMA_REDIRECT)
> +				acs_redirect_cnt++;
> +			else if (!acs_unsupported)
> +				acs_unsupported = bb;
>  		}
>  
>  		bb = pci_upstream_bridge(bb);

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