| Message ID | 20261001-fix-p2p-acs-v4-0-v9-4-1a8e0f50ddd9@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/P2PDMA: Route peer-to-peer DMA by TLP class | expand |
> From: Leon Romanovsky <leonro@nvidia.com> > > ACS redirect controls choose between peer and upstream routes only at the > path divergence. Applying them below that point rejects valid nested > topologies because traffic already has only an upstream route. > > Evaluate Request controls on the client-side divergence port and Completion > Redirect on the provider-side port and reject an unreadable ACS Control > register. > > Fixes: 52916982af48 ("PCI/P2PDMA: Support peer-to-peer memory") > Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > Tested-by: Tushar Dave <tdave@nvidia.com> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> Sashiko has reviewed this patch and found no issues. It looks great!
On Thu, Oct 01, 2026 at 02:55:12PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > ACS redirect controls choose between peer and upstream routes only at the > path divergence. Applying them below that point rejects valid nested > topologies because traffic already has only an upstream route. I guess the point here is that prior to this patch, calc_map_type_and_dist() returned PCI_P2PDMA_MAP_NOT_SUPPORTED in a case where it didn't need to? Can you include an example to make this concrete? It looks like in v7.3, we only return PCI_P2PDMA_MAP_NOT_SUPPORTED if a TLP has to go through a host bridge. Do we mistakenly assume that if a bridge has PCI_ACS_RR set, a Request must go all the way to the host bridge, even if a bridge closer to the root does not have PCI_ACS_RR set? > Evaluate Request controls on the client-side divergence port and Completion > Redirect on the provider-side port and reject an unreadable ACS Control > register. > > Fixes: 52916982af48 ("PCI/P2PDMA: Support peer-to-peer memory") > Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > Tested-by: Tushar Dave <tdave@nvidia.com> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > --- > drivers/pci/p2pdma.c | 101 +++++++++++++++++++++++++++++---------------- > include/linux/pci-p2pdma.h | 8 ++-- > 2 files changed, 70 insertions(+), 39 deletions(-) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index 12612b82d80d..550e6c7346ef 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -493,6 +493,7 @@ static struct pci_dev *find_parent_pci_dev(struct device *dev) > } > > enum pci_acs_p2pdma_state { > + PCI_ACS_P2PDMA_NOT_SUPPORTED, > PCI_ACS_P2PDMA_DIRECT, > PCI_ACS_P2PDMA_REDIRECT, > }; > @@ -730,13 +731,13 @@ static unsigned long map_types_idx(struct pci_dev *client) > * then to Device B. The mapping type returned depends on the ACS > * redirection setting of the ports along the path. > * > - * The client initiates Requests to provider memory. Check Request Redirect > - * on the client path and Completion Redirect for read Completions on the > - * provider path. > + * The client initiates Requests to provider memory. At the path divergence, > + * check Request Redirect and Egress Control on the client-side port, and > + * Completion Redirect for read Completions on the provider-side port. > * > - * If ACS redirect is set on any port in the path, traffic between the > - * devices will go through the host bridge, so return > - * PCI_P2PDMA_MAP_THRU_HOST_BRIDGE; otherwise return > + * If ACS redirects traffic at either divergence port, return > + * PCI_P2PDMA_MAP_THRU_HOST_BRIDGE. If the ACS Control register cannot be > + * read, return PCI_P2PDMA_MAP_NOT_SUPPORTED. Otherwise, return > * PCI_P2PDMA_MAP_BUS_ADDR. > * > * Any two devices that have a data path that goes through the host bridge > @@ -750,10 +751,13 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, > int *dist, bool verbose) > { > enum pci_p2pdma_map_type map_type = PCI_P2PDMA_MAP_THRU_HOST_BRIDGE; > + enum pci_acs_p2pdma_state state = PCI_ACS_P2PDMA_NOT_SUPPORTED; > struct pci_dev *a = provider, *b = client, *bb; > + struct pci_dev *a_child = NULL, *b_child = NULL; > + struct pci_dev *acs_unreadable = NULL; > struct pci_p2pdma *p2pdma; > struct seq_buf acs_list; > - int acs_cnt = 0; > + int acs_redirect_cnt = 0; > int dist_a = 0; > int dist_b = 0; > char buf[128]; > @@ -768,51 +772,67 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, > */ > while (a) { > dist_b = 0; > - > - if (!pci_acs_p2pdma_ctrl(a, &ctrl) || > - pci_acs_p2pdma_completion(ctrl) == > - PCI_ACS_P2PDMA_REDIRECT) { > - seq_buf_print_bus_devfn(&acs_list, a); > - acs_cnt++; > - } > - > + b_child = NULL; > bb = b; > > while (bb) { > if (a == bb) > - goto check_b_path_acs; > + goto check_paths_acs; > > + b_child = bb; > bb = pci_upstream_bridge(bb); > dist_b++; > } > > + a_child = a; > a = pci_upstream_bridge(a); > dist_a++; > } > > + /* > + * The paths share no upstream bridge, so there is no direct path for > + * ACS to gate: PCI_P2PDMA_MAP_BUS_ADDR is not reachable here and the > + * request can only get to the peer through the host bridge. > + */ > *dist = dist_a + dist_b; > goto map_through_host_bridge; > > -check_b_path_acs: > - bb = b; > - > - while (bb) { > - if (a == bb) > - break; > +check_paths_acs: > + *dist = dist_a + dist_b; > > - if (!pci_acs_p2pdma_ctrl(bb, &ctrl) || > - pci_acs_p2pdma_request(ctrl) == > - PCI_ACS_P2PDMA_REDIRECT) { > - seq_buf_print_bus_devfn(&acs_list, bb); > - acs_cnt++; > + /* > + * ACS P2P routing controls apply where a TLP can route toward the peer > + * or upstream. Below that divergence, its only route toward the other > + * branch is upstream, so redirect controls do not affect the path. > + */ > + if (a_child && b_child) { > + if (pci_acs_p2pdma_ctrl(a_child, &ctrl)) > + state = pci_acs_p2pdma_completion(ctrl); > + if (state != PCI_ACS_P2PDMA_DIRECT) { > + seq_buf_print_bus_devfn(&acs_list, a_child); > + if (state == PCI_ACS_P2PDMA_REDIRECT) > + acs_redirect_cnt++; > + else if (!acs_unreadable) > + acs_unreadable = a_child; > } > > - bb = pci_upstream_bridge(bb); > + state = PCI_ACS_P2PDMA_NOT_SUPPORTED; > + if (pci_acs_p2pdma_ctrl(b_child, &ctrl)) > + state = pci_acs_p2pdma_request(ctrl); > + if (state != PCI_ACS_P2PDMA_DIRECT) { > + seq_buf_print_bus_devfn(&acs_list, b_child); > + if (state == PCI_ACS_P2PDMA_REDIRECT) > + acs_redirect_cnt++; > + else if (!acs_unreadable) > + acs_unreadable = b_child; > + } > } > > - *dist = dist_a + dist_b; > - > - if (!acs_cnt) { > + /* > + * Below a shared upstream bridge, a path whose divergence ports do not > + * redirect routes the request directly. > + */ > + if (!acs_unreadable && !acs_redirect_cnt) { > map_type = PCI_P2PDMA_MAP_BUS_ADDR; > goto done; > } > @@ -821,10 +841,21 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, > /* Drop the final semicolon; the list is not empty here. */ > if (!seq_buf_has_overflowed(&acs_list)) > acs_list.buffer[acs_list.len - 1] = '\0'; > - pci_warn(client, "ACS redirect is set between the client and provider (%s)\n", > - pci_name(provider)); > - pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n", > - seq_buf_str(&acs_list)); > + if (acs_unreadable) > + pci_warn(client, "ACS Control is unreadable for provider %s at %s\n", > + pci_name(provider), pci_name(acs_unreadable)); > + else { > + pci_warn(client, "ACS redirect is set between the client and provider (%s)\n", > + pci_name(provider)); > + pci_warn(client, "to disable ACS controls for this path, add the kernel parameter: pci=disable_acs_redir=%s\n", > + seq_buf_str(&acs_list)); > + } > + } > + > + /* An unreadable control does not establish an upstream redirect. */ > + if (acs_unreadable) { > + map_type = PCI_P2PDMA_MAP_NOT_SUPPORTED; > + goto done; > } > > map_through_host_bridge: > diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h > index 873de20a2247..dd17501ba1b6 100644 > --- a/include/linux/pci-p2pdma.h > +++ b/include/linux/pci-p2pdma.h > @@ -42,10 +42,10 @@ enum pci_p2pdma_map_type { > PCI_P2PDMA_MAP_NONE, > > /* > - * PCI_P2PDMA_MAP_NOT_SUPPORTED: Indicates the transaction will > - * traverse the host bridge and the host bridge is not in the > - * allowlist. DMA Mapping routines should return an error when > - * this is returned. > + * PCI_P2PDMA_MAP_NOT_SUPPORTED: Indicates no safe mapping is available, > + * for example because ACS blocks the direct path or the required host > + * bridge is not in the allowlist. DMA Mapping routines should return an > + * error when this is returned. > */ > PCI_P2PDMA_MAP_NOT_SUPPORTED, > > > -- > 2.55.0 >
On Tue, Oct 06, 2026 at 04:08:48PM -0500, Bjorn Helgaas wrote: > On Thu, Oct 01, 2026 at 02:55:12PM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > ACS redirect controls choose between peer and upstream routes only at the > > path divergence. Applying them below that point rejects valid nested > > topologies because traffic already has only an upstream route. > > I guess the point here is that prior to this patch, > calc_map_type_and_dist() returned PCI_P2PDMA_MAP_NOT_SUPPORTED in a > case where it didn't need to? Can you include an example to make this > concrete? One possible example I had in mind while preparing the presentation [1] was ability to connect devices to different switches. Despite devices and switches support P2P, but from the P2P subsystem's point of view, traffic between them was not P2P (see slide 4). This support will become even more important when we reach the point of enabling P2P inside a VM. It will allow us to hide the underlying PCI topology from the VM. Thanks [1] http://lpc.events/event/20/contributions/2508/
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index 12612b82d80d..550e6c7346ef 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -493,6 +493,7 @@ static struct pci_dev *find_parent_pci_dev(struct device *dev) } enum pci_acs_p2pdma_state { + PCI_ACS_P2PDMA_NOT_SUPPORTED, PCI_ACS_P2PDMA_DIRECT, PCI_ACS_P2PDMA_REDIRECT, }; @@ -730,13 +731,13 @@ static unsigned long map_types_idx(struct pci_dev *client) * then to Device B. The mapping type returned depends on the ACS * redirection setting of the ports along the path. * - * The client initiates Requests to provider memory. Check Request Redirect - * on the client path and Completion Redirect for read Completions on the - * provider path. + * The client initiates Requests to provider memory. At the path divergence, + * check Request Redirect and Egress Control on the client-side port, and + * Completion Redirect for read Completions on the provider-side port. * - * If ACS redirect is set on any port in the path, traffic between the - * devices will go through the host bridge, so return - * PCI_P2PDMA_MAP_THRU_HOST_BRIDGE; otherwise return + * If ACS redirects traffic at either divergence port, return + * PCI_P2PDMA_MAP_THRU_HOST_BRIDGE. If the ACS Control register cannot be + * read, return PCI_P2PDMA_MAP_NOT_SUPPORTED. Otherwise, return * PCI_P2PDMA_MAP_BUS_ADDR. * * Any two devices that have a data path that goes through the host bridge @@ -750,10 +751,13 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, int *dist, bool verbose) { enum pci_p2pdma_map_type map_type = PCI_P2PDMA_MAP_THRU_HOST_BRIDGE; + enum pci_acs_p2pdma_state state = PCI_ACS_P2PDMA_NOT_SUPPORTED; struct pci_dev *a = provider, *b = client, *bb; + struct pci_dev *a_child = NULL, *b_child = NULL; + struct pci_dev *acs_unreadable = NULL; struct pci_p2pdma *p2pdma; struct seq_buf acs_list; - int acs_cnt = 0; + int acs_redirect_cnt = 0; int dist_a = 0; int dist_b = 0; char buf[128]; @@ -768,51 +772,67 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, */ while (a) { dist_b = 0; - - if (!pci_acs_p2pdma_ctrl(a, &ctrl) || - pci_acs_p2pdma_completion(ctrl) == - PCI_ACS_P2PDMA_REDIRECT) { - seq_buf_print_bus_devfn(&acs_list, a); - acs_cnt++; - } - + b_child = NULL; bb = b; while (bb) { if (a == bb) - goto check_b_path_acs; + goto check_paths_acs; + b_child = bb; bb = pci_upstream_bridge(bb); dist_b++; } + a_child = a; a = pci_upstream_bridge(a); dist_a++; } + /* + * The paths share no upstream bridge, so there is no direct path for + * ACS to gate: PCI_P2PDMA_MAP_BUS_ADDR is not reachable here and the + * request can only get to the peer through the host bridge. + */ *dist = dist_a + dist_b; goto map_through_host_bridge; -check_b_path_acs: - bb = b; - - while (bb) { - if (a == bb) - break; +check_paths_acs: + *dist = dist_a + dist_b; - if (!pci_acs_p2pdma_ctrl(bb, &ctrl) || - pci_acs_p2pdma_request(ctrl) == - PCI_ACS_P2PDMA_REDIRECT) { - seq_buf_print_bus_devfn(&acs_list, bb); - acs_cnt++; + /* + * ACS P2P routing controls apply where a TLP can route toward the peer + * or upstream. Below that divergence, its only route toward the other + * branch is upstream, so redirect controls do not affect the path. + */ + if (a_child && b_child) { + if (pci_acs_p2pdma_ctrl(a_child, &ctrl)) + state = pci_acs_p2pdma_completion(ctrl); + if (state != PCI_ACS_P2PDMA_DIRECT) { + seq_buf_print_bus_devfn(&acs_list, a_child); + if (state == PCI_ACS_P2PDMA_REDIRECT) + acs_redirect_cnt++; + else if (!acs_unreadable) + acs_unreadable = a_child; } - bb = pci_upstream_bridge(bb); + state = PCI_ACS_P2PDMA_NOT_SUPPORTED; + if (pci_acs_p2pdma_ctrl(b_child, &ctrl)) + state = pci_acs_p2pdma_request(ctrl); + if (state != PCI_ACS_P2PDMA_DIRECT) { + seq_buf_print_bus_devfn(&acs_list, b_child); + if (state == PCI_ACS_P2PDMA_REDIRECT) + acs_redirect_cnt++; + else if (!acs_unreadable) + acs_unreadable = b_child; + } } - *dist = dist_a + dist_b; - - if (!acs_cnt) { + /* + * Below a shared upstream bridge, a path whose divergence ports do not + * redirect routes the request directly. + */ + if (!acs_unreadable && !acs_redirect_cnt) { map_type = PCI_P2PDMA_MAP_BUS_ADDR; goto done; } @@ -821,10 +841,21 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, /* Drop the final semicolon; the list is not empty here. */ if (!seq_buf_has_overflowed(&acs_list)) acs_list.buffer[acs_list.len - 1] = '\0'; - pci_warn(client, "ACS redirect is set between the client and provider (%s)\n", - pci_name(provider)); - pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n", - seq_buf_str(&acs_list)); + if (acs_unreadable) + pci_warn(client, "ACS Control is unreadable for provider %s at %s\n", + pci_name(provider), pci_name(acs_unreadable)); + else { + pci_warn(client, "ACS redirect is set between the client and provider (%s)\n", + pci_name(provider)); + pci_warn(client, "to disable ACS controls for this path, add the kernel parameter: pci=disable_acs_redir=%s\n", + seq_buf_str(&acs_list)); + } + } + + /* An unreadable control does not establish an upstream redirect. */ + if (acs_unreadable) { + map_type = PCI_P2PDMA_MAP_NOT_SUPPORTED; + goto done; } map_through_host_bridge: diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h index 873de20a2247..dd17501ba1b6 100644 --- a/include/linux/pci-p2pdma.h +++ b/include/linux/pci-p2pdma.h @@ -42,10 +42,10 @@ enum pci_p2pdma_map_type { PCI_P2PDMA_MAP_NONE, /* - * PCI_P2PDMA_MAP_NOT_SUPPORTED: Indicates the transaction will - * traverse the host bridge and the host bridge is not in the - * allowlist. DMA Mapping routines should return an error when - * this is returned. + * PCI_P2PDMA_MAP_NOT_SUPPORTED: Indicates no safe mapping is available, + * for example because ACS blocks the direct path or the required host + * bridge is not in the allowlist. DMA Mapping routines should return an + * error when this is returned. */ PCI_P2PDMA_MAP_NOT_SUPPORTED,