Re: [PATCH v3] iommu/riscv: Use 32-bit MMIO accesses for 64-bit registers

[David Laight <david.laight.linux@gmail.com>]

On Wed, 15 Jul 2026 00:53:06 +0800
Guo Ren <guoren@kernel.org> wrote:

> On Tue, Jul 14, 2026 at 10:53 PM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Tue, 14 Jul 2026 21:24:19 +0800
> > Guo Ren <guoren@kernel.org> wrote:
> >  
> > > On Tue, Jul 14, 2026 at 8:27 PM Robin Murphy <robin.murphy@arm.com> wrote:  
> > > >
> > > > On 13/07/2026 1:29 pm, Zhanpeng Zhang wrote:  
> > > > > The RISC-V IOMMU specification [1] permits 64-bit registers to be accessed
> > > > > using two 32-bit transactions, high half first, and leaves the single-copy
> > > > > atomicity of 8-byte IOMMU register accesses unspecified.
> > > > >
> > > > > Use the generic hi_lo_readq_relaxed() and hi_lo_writeq_relaxed() helpers
> > > > > for ordinary 64-bit IOMMU registers. For DDTP, poll BUSY in the low half,
> > > > > then read the high half and compose the register value from the polled low
> > > > > half. HPM counter reads require a rollover-aware sequence and remain
> > > > > outside these accessors.
> > > > >
> > > > > This follows the 32-bit access direction proposed by Guo Ren [2] and uses
> > > > > the generic non-atomic MMIO helpers suggested by David Laight.
> > > > >
> > > > > [1] https://docs.riscv.org/reference/iommu/
> > > > > [2] https://lore.kernel.org/r/20250903144217.837448-1-guoren@kernel.org
> > > > >
> > > > > Suggested-by: Guo Ren <guoren@kernel.org>
> > > > > Suggested-by: David Laight <david.laight.linux@gmail.com>
> > > > > Signed-off-by: Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com>
> > > > > ---
> > > > > Changes in v3:
> > > > > - Use the DDTP access sequence from [1]: retain the low half returned by
> > > > >    BUSY polling, read only the high half, and compose the DDTP value from
> > > > >    those two 32-bit reads.
> > > > >
> > > > > Changes in v2:
> > > > > - Rework the patch based on Guo Ren's earlier proposal [1].
> > > > > - Drop the build-time option and use 32-bit accesses unconditionally.
> > > > > - Drop the global lock and use the generic high-low MMIO helpers, as
> > > > >    suggested by David Laight.
> > > > > - Poll DDTP.BUSY through its low half.
> > > > >
> > > > > Link to v1: [2]
> > > > > Specification discussion: [3]
> > > > >
> > > > > [1]: https://lore.kernel.org/r/20250903144217.837448-1-guoren@kernel.org
> > > > > [2]: https://lore.kernel.org/r/20260615064855.90316-1-zhangzhanpeng.jasper@bytedance.com
> > > > > [3]: https://github.com/riscv-non-isa/riscv-iommu/issues/765
> > > > >
> > > > >   drivers/iommu/riscv/iommu.c | 9 ++++++---
> > > > >   drivers/iommu/riscv/iommu.h | 9 +++------
> > > > >   2 files changed, 9 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> > > > > index cec3ddd7ab1..d647b71ebec 100644
> > > > > --- a/drivers/iommu/riscv/iommu.c
> > > > > +++ b/drivers/iommu/riscv/iommu.c
> > > > > @@ -670,9 +670,12 @@ void riscv_iommu_disable(struct riscv_iommu_device *iommu)
> > > > >
> > > > >   #define riscv_iommu_read_ddtp(iommu) ({ \
> > > > >       u64 ddtp; \
> > > > > -     riscv_iommu_readq_timeout((iommu), RISCV_IOMMU_REG_DDTP, ddtp, \
> > > > > -                               !(ddtp & RISCV_IOMMU_DDTP_BUSY), 10, \
> > > > > +     u32 ddtp_lo, ddtp_hi; \
> > > > > +     riscv_iommu_readl_timeout((iommu), RISCV_IOMMU_REG_DDTP, ddtp_lo, \
> > > > > +                               !(ddtp_lo & RISCV_IOMMU_DDTP_BUSY), 10, \
> > > > >                                 RISCV_IOMMU_DDTP_TIMEOUT); \
> > > > > +     ddtp_hi = riscv_iommu_readl((iommu), RISCV_IOMMU_REG_DDTP + 4); \  
> > > >
> > > > It looks like whenever you read DDTP you're only really looking at the
> > > > BUSY/MODE fields anyway, so does this actually need to read the upper
> > > > bits of PPN at all? (The spec says they don't even need to be written on
> > > > RV32 either)  
> > >
> > > The motivation is not RV32 compatibility. It addresses RV64 platforms
> > > where the IOMMU control plane can only be accessed via 32-bit MMIO.
> > >  
> > > >  
> > > > > +     ddtp = ((u64)ddtp_hi << 32) | ddtp_lo; \
> > > > >       ddtp; })
> > > > >
> > > > >   static int riscv_iommu_iodir_alloc(struct riscv_iommu_device *iommu)
> > > > > @@ -1501,7 +1504,7 @@ static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
> > > > >        * regular boot flow and disable translation when we boot into a kexec
> > > > >        * kernel and the previous kernel left them enabled.
> > > > >        */
> > > > > -     ddtp = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_DDTP);
> > > > > +     ddtp = riscv_iommu_read_ddtp(iommu);
> > > > >       if (ddtp & RISCV_IOMMU_DDTP_BUSY)
> > > > >               return -EBUSY;
> > > > >
> > > > > diff --git a/drivers/iommu/riscv/iommu.h b/drivers/iommu/riscv/iommu.h
> > > > > index 46df79dd549..1b03790fbe1 100644
> > > > > --- a/drivers/iommu/riscv/iommu.h
> > > > > +++ b/drivers/iommu/riscv/iommu.h
> > > > > @@ -11,6 +11,7 @@
> > > > >   #ifndef _RISCV_IOMMU_H_
> > > > >   #define _RISCV_IOMMU_H_
> > > > >
> > > > > +#include <linux/io-64-nonatomic-hi-lo.h>
> > > > >   #include <linux/iommu.h>
> > > > >   #include <linux/types.h>
> > > > >   #include <linux/iopoll.h>
> > > > > @@ -70,17 +71,13 @@ void riscv_iommu_disable(struct riscv_iommu_device *iommu);
> > > > >       readl_relaxed((iommu)->reg + (addr))
> > > > >
> > > > >   #define riscv_iommu_readq(iommu, addr) \
> > > > > -     readq_relaxed((iommu)->reg + (addr))
> > > > > +     hi_lo_readq_relaxed((iommu)->reg + (addr))  
> > > >
> > > > This seems unnecessary (similarly for writeq() below) - once you've
> > > > included the header, then it automatically provides readq{_relaxed}()
> > > > for RV32 via the non-atomic implementation, and wherever atomicity
> > > > doesn't matter, then as written there seems to be no reason for RV64 to
> > > > stop using the regular arch readq().
> > > >
> > > > Not that it makes any difference to me either way, but I don't see any
> > > > real issue with the spec - if software *may* make a 64-bit access to any
> > > > 64-bit register unconditionally, then that can only imply that hardware
> > > > *must* be able to accommodate RV64 software choosing to do so, and
> > > > therefore must support *both* 32b and 64b accesses in general, except
> > > > perhaps on RV32-only systems if software could never make 64b accesses
> > > > in the first place. If 64b single-copy atomicity is not
> > > > required/specified then it should be valid to achieve that by just
> > > > sticking a downsizer in the upstream interconnect to split 64b accesses
> > > > into 32bx2 incremental bursts.
> > > >
> > > > If do actually you need this as an erratum workaround to support
> > > > specific hardware which has misinterpreted the spec then I think you
> > > > should be clear about that. Otherwise, adding speculative "workarounds"
> > > > which (slightly) penalise hardware that got it right, while inviting
> > > > future hardware to get it wrong, doesn't seem like the right way to go
> > > > at all...  
> > >
> > > The only three 64-bit registers the driver ever touches are DDTP,
> > > CAPABILITIES, and MSI_CFG_TBL_ADDR. None of them is on any
> > > performance-critical or hot path. Even if native 64-bit MMIO accesses
> > > were used, there would be no observable performance benefit.
> > > Therefore, this change does not penalize any hardware.
> > >
> > > Regarding the specification, there remains substantial disagreement
> > > over whether 64-bit accesses are mandatory. The relevant sentence uses
> > > the word “may” (“may be accessed using either a 32-bit or a 64-bit
> > > access”). Per RFC 2119, “may” does not express a requirement. Because
> > > every existing implementation already provides working 32-bit MMIO
> > > access, we believe the safer and less controversial choice is to use
> > > the portable 32-bit path via the generic hi-lo helpers.
> > >  
> >
> > There is also this clause:
> >         The 8-byte IOMMU registers are defined in such a way that software can
> >         perform two individual 4-byte accesses, or hardware can perform two
> >         independent 4-byte transactions resulting from an 8-byte access,
> >         to the high and low halves of the register, in that order, as long as
> >         the register semantics, with regard to side-effects, are respected
> >         between the two software accesses, or two hardware transactions,
> >         respectively.
> >
> > I'm not sure what the relevance of the high-low order is.
> > It could mean, for example, that a read of the high word can latch the low
> > for the next read - so that you get an atomic 64bit read.
> > That might be ok if hardware splits the 64bit read, but if software does it
> > it isn't going to go well unless the driver locks all (and it might be all)
> > slave accesses.
> > In the worst case there might be a single latch for all odd word reads.
> > Writes could be worse!
> >
> > Hardware can be that broken.
> > I remember a PCI slave that terminated reads requesting 'cycle rerun'
> > and then assumed that the following reads would be repeats of the same
> > read, not reads of a different address by the other cpu.
> > It didn't do writes properly either, I've forgotten the exact details
> > but I think a second (third?) write overwrite some latches.  
> 
> This is precisely why the specification leaves the single-copy
> atomicity of an 8-byte IOMMU register access UNSPECIFIED. If a 64-bit
> MMIO register were implemented with atomic semantics, 32-bit accesses
> using lw/sw might require implicit mechanisms, such as latching half
> the register for subsequent accesses. Such mechanisms could lead to
> exactly the kind of interleaving problem you described.

Right, but it also says you must do high-low.
The only reason for that would be to allow hardware that requires
that ordering - which means the software better do both accesses
in that order.
The second half of the sentence seems to let the hardware update
half the register from a 32bit access (as you might expect).

The whole thing is misleading/confusing.
And, reading the full page, the system that doesn't support 64bit
accesses is probably non-conformant as well.

	David


> 
> This is one reason hardware may favor straightforward, latch-free
> 32-bit MMIO transactions. The example you mentioned further
> illustrates the robustness of treating each 32-bit MMIO access as an
> independent transaction without relying on hidden latch semantics.
> 
> Using 32-bit MMIO accesses for DDTP, CAPABILITIES, and
> MSI_CFG_TBL_ADDR in this driver introduces no meaningful performance
> overhead and provides broader compatibility without disadvantaging any
> RISC-V IOMMU implementation.
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv