Re: [PATCH v6 18/18] RDMA/mlx5: Ask P2PDMA whether ATS takes a direct peer-to-peer route

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Leon,

On Fri, 2026-09-18 at 15:15 +0300, Leon Romanovsky wrote:
> On Thu, Sep 17, 2026 at 03:56:12PM +0200, Thomas Hellström wrote:
> > Hi
> > 
> > On Mon, 2026-09-14 at 14:22 +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > mlx5_umem_needs_ats() enables ATS for any dma-buf whose caller
> > > asked
> > > for
> > > Relaxed Ordering, on the assumption that a switch in the path has
> > > CR,
> > > RR
> > > and DT all set. It also enables it for a buffer already mapped
> > > with
> > > the
> > > peer's bus addresses, which are not translatable at all.
> > > 
> > > P2PDMA has read the ACS controls, so ask it through
> > > dma_buf_p2pdma_map_type(): enable ATS only where the path is not
> > > routed
> > > directly as it stands, but would be for a Translated Request
> > > whose
> > > Completions carry Relaxed Ordering. Exporters that name no
> > > provider
> > > keep
> > > the old assumption, since their ACS settings remain hidden.
> > > 
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > ---
> > >  drivers/infiniband/hw/mlx5/mlx5_ib.h | 36 ++--------------------
> > > ----
> > > ------
> > >  drivers/infiniband/hw/mlx5/mr.c      | 40
> > > ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 42 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > index e9ddf2e97a76..ab32742b2180 100644
> > > --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > @@ -1646,40 +1646,8 @@ static inline bool rt_supported(int
> > > ts_cap)
> > >  	       ts_cap ==
> > > MLX5_TIMESTAMP_FORMAT_CAP_FREE_RUNNING_AND_REAL_TIME;
> > >  }
> > >  
> > > -/*
> > > - * PCI Peer to Peer is a trainwreck. If no switch is present
> > > then
> > > things
> > > - * sometimes work, depending on the pci_distance_p2p logic for
> > > excluding broken
> > > - * root complexes. However if a switch is present in the path,
> > > then
> > > things get
> > > - * really ugly depending on how the switch is setup. This table
> > > assumes that the
> > > - * root complex is strict and is validating that all req/reps
> > > are
> > > matches
> > > - * perfectly - so any scenario where it sees only half the
> > > transaction is a
> > > - * failure.
> > > - *
> > > - * CR/RR/DT  ATS RO P2P
> > > - * 00X       X   X  OK
> > > - * 010       X   X  fails (request is routed to root but root
> > > never
> > > sees comp)
> > > - * 011       0   X  fails (request is routed to root but root
> > > never
> > > sees comp)
> > > - * 011       1   X  OK
> > > - * 10X       X   1  OK
> > > - * 101       X   0  fails (completion is routed to root but root
> > > didn't see req)
> > > - * 110       X   0  SLOW
> > > - * 111       0   0  SLOW
> > > - * 111       1   0  fails (completion is routed to root but root
> > > didn't see req)
> > > - * 111       1   1  OK
> > > - *
> > > - * Unfortunately we cannot reliably know if a switch is present
> > > or
> > > what the
> > > - * CR/RR/DT ACS settings are, as in a VM that is all hidden.
> > > Assume
> > > that
> > > - * CR/RR/DT is 111 if the ATS cap is enabled and follow the last
> > > three rows.
> > > - *
> > > - * For now assume if the umem is a dma_buf then it is P2P.
> > > - */
> > > -static inline bool mlx5_umem_needs_ats(struct mlx5_ib_dev *dev,
> > > -				       struct ib_umem *umem, int
> > > access_flags)
> > > -{
> > > -	if (!MLX5_CAP_GEN(dev->mdev, ats) || !umem->is_dmabuf)
> > > -		return false;
> > > -	return access_flags & IB_ACCESS_RELAXED_ORDERING;
> > > -}
> > > +bool mlx5_umem_needs_ats(struct mlx5_ib_dev *dev, struct ib_umem
> > > *umem,
> > > +			 int access_flags);
> > >  
> > >  int set_roce_addr(struct mlx5_ib_dev *dev, u32 port_num,
> > >  		  unsigned int index, const union ib_gid *gid,
> > > diff --git a/drivers/infiniband/hw/mlx5/mr.c
> > > b/drivers/infiniband/hw/mlx5/mr.c
> > > index 00e13028762a..286f372e5b0c 100644
> > > --- a/drivers/infiniband/hw/mlx5/mr.c
> > > +++ b/drivers/infiniband/hw/mlx5/mr.c
> > > @@ -38,6 +38,7 @@
> > >  #include <linux/export.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/dma-buf.h>
> > > +#include <linux/dma-buf-mapping.h>
> > >  #include <linux/dma-resv.h>
> > >  #include <rdma/frmr_pools.h>
> > >  #include <rdma/ib_umem_odp.h>
> > > @@ -47,6 +48,45 @@
> > >  #include "data_direct.h"
> > >  #include "dmah.h"
> > >  
> > > +MODULE_IMPORT_NS("DMA_BUF");
> > > +
> > > +bool mlx5_umem_needs_ats(struct mlx5_ib_dev *dev, struct ib_umem
> > > *umem,
> > > +			 int access_flags)
> > > +{
> > > +	struct dma_buf_attachment *attach;
> > > +
> > > +	if (!MLX5_CAP_GEN(dev->mdev, ats) || !umem->is_dmabuf)
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * The Completer decides whether its Completions carry
> > > Relaxed
> > > +	 * Ordering, and only a Request that asked for it can
> > > expect
> > > them to.
> > > +	 */
> > > +	if (!(access_flags & IB_ACCESS_RELAXED_ORDERING))
> > > +		return false;
> > > +
> > > +	attach = to_ib_umem_dmabuf(umem)->attach;
> > > +	switch (dma_buf_p2pdma_map_type(attach, 0)) {
> > > +	case PCI_P2PDMA_MAP_NONE:
> > > +		/* Nothing is known about the route, so fall
> > > back to
> > > the bet. */
> > > +		return true;
> > > +	case PCI_P2PDMA_MAP_BUS_ADDR:
> > > +		/*
> > > +		 * The path is routed directly already and is
> > > programmed with
> > > +		 * the peer's bus addresses. Those are not
> > > translatable, so
> > > +		 * ATS would be wrong as well as pointless.
> > > +		 */
> > > +		return false;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	return dma_buf_p2pdma_map_type(attach,
> > > +				       PCI_P2PDMA_TLP_TRANSLATED
> > > |
> > > +					      
> > > PCI_P2PDMA_TLP_RELAXED_CPL) ==
> > > +	       PCI_P2PDMA_MAP_BUS_ADDR;
> > > +}
> > > +
> > 
> > It looks like this works well when the device can choose whether to
> > enable ATS per transaction. 
> > 
> > However, at least for the Intel GPUs, ATS enablement is based on
> > the
> > PCIe-side enable bit.
> > 
> > This means that if p2pdma tells the dma-mapping layer to give an Xe
> > device a bus address rather than an IOVA, things break, while
> > pci_p2pdma_distance says everything is OK.
> > 
> > It looks like the infrastructure and solution added in this series
> > is
> > targeted at fixing this on the device side by conditionally
> > enabling
> > ATS. However I think we need to look also at having the computed
> > routing assume untranslated transactions using IOVA rather than bus
> > address.
> > 
> > That is, a flag to tell the topology check that some transactions
> > *will* take the host-bridge path due to IOVA being used, and that
> > the
> > computations including pci_p2pdma_distance() need to check whether
> > that
> > is possible (checking whitelist etc.) and return the corresponding
> > THRU_HOST_BRIDGE mapping type. Translated transactions taking a
> > short-
> > cut using the bus-address would then be hidden from the driver.
> 
> The word *will* puzzles me. As I understand it, if your device has
> ATS
> enabled all the time, it should always get THRU_HOST_BRIDGE.
> 
> In the mlx5 case, we can enable or disable ATS on the fly, but what
> does a
> device with ATS enabled globally do for different mapping types? Do
> you
> simply not use dma_buf_p2pdma_map_type() at all and continue to
> operate
> as is?
> 
> I asked some questions my AI tool about XE:
> ---------------------------------------------------------------------
> ---------------------------
> Xe needs THRU_HOST_BRIDGE even with ATS completely disabled.
> xe_ttm_vram_mgr_alloc_sgt()
> maps VRAM with dma_map_resource() unconditionally
> (xe_ttm_vram_mgr.c:450) — an IOVA under
> an IOMMU, never a bus address, with no ATS involvement at all. ATS
> only adds the possibility
> that some of that traffic short-cuts invisibly. The actual
> requirement is “this caller cannot
> program bus addresses”, which is true of Xe regardless of the ATS
> Enable bit.
> ---------------------------------------------------------------------
> ----------------------------
> 
> Could you please clarify what the expected behavior is here? My
> series
> does not change the existing behavior; it only reports the routing
> more
> clearly for each TLP class.
> 
> Thanks

So the *current* concerns are these (assuming I haven't misunderstood
the code).

1) Xe attachment check if pci_p2pdma_distance() returns OK for the
path. Then Xe always sets up dma-addresses using dma_map_resource(). If
IOMMU is enabled, that's always the IOMMU VAs. But if the host bridge
is not whitelisted, and pci_p2pdma_distance() has found a shortcut that
works using BUS_ADDR, then p2pdma is incorrectly enabled, and will be
routed using a non-supporting host bridge.

2) If an ATS on/off device (outside of dma-buf) were to map ZONE_DEVICE
pci_p2pdma pages using dma_map_sgtable(), then again the dma addresses
returned may be BUS_ADDR addresses, if the topology check finds a
direct path. But if ATS is on, then this also breaks down.

Now after your patch series, I tried to set up a patch for xe and ran
into the following:

1a) Let's say the importer (Xe) has ATS enabled, and the exporter is
supposed to map dma_addresses.
In between there is a switch that allows direct traffic.
Now with your patch series, How would the exporter know that Xe has ATS
enabled and therefore should return IOVA mappings rather than the BUS
addresses representing the shortcut.

2a) Does your patch series resolve that potential issue?

It seems to me that a pci-device settable flag "ATS always enabled"
should be enough to fix both issues?

Thanks,
Thomas


> 
> > 
> > Whether that is best done as a parameter to these functions or
> > perhaps
> > as a flag in the PCI device, I'm not sure.
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > 
> > 
> > >  static int mkey_max_umr_order(struct mlx5_ib_dev *dev)
> > >  {
> > >  	if (MLX5_CAP_GEN(dev->mdev,
> > > umr_extended_translation_offset))