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); \
+	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))
 
 #define riscv_iommu_writel(iommu, addr, val) \
 	writel_relaxed((val), (iommu)->reg + (addr))
 
 #define riscv_iommu_writeq(iommu, addr, val) \
-	writeq_relaxed((val), (iommu)->reg + (addr))
-
-#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
-	readx_poll_timeout(readq_relaxed, (iommu)->reg + (addr), val, cond, \
-			   delay_us, timeout_us)
+	hi_lo_writeq_relaxed((val), (iommu)->reg + (addr))
 
 #define riscv_iommu_readl_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
 	readx_poll_timeout(readl_relaxed, (iommu)->reg + (addr), val, cond, \
-- 
2.50.1 (Apple Git-155)

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)

> +	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...

Thanks,
Robin.

>   
>   #define riscv_iommu_writel(iommu, addr, val) \
>   	writel_relaxed((val), (iommu)->reg + (addr))
>   
>   #define riscv_iommu_writeq(iommu, addr, val) \
> -	writeq_relaxed((val), (iommu)->reg + (addr))
> -
> -#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> -	readx_poll_timeout(readq_relaxed, (iommu)->reg + (addr), val, cond, \
> -			   delay_us, timeout_us)
> +	hi_lo_writeq_relaxed((val), (iommu)->reg + (addr))
>   
>   #define riscv_iommu_readl_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
>   	readx_poll_timeout(readl_relaxed, (iommu)->reg + (addr), val, cond, \

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)
>
> > +     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

You cannot exclude RV32 from the discussion. The spec statement in question is:
"Registers that are 64-bit wide may be accessed using either a 32-bit
or a 64-bit access."

This sentence applies to all implementations, including RV32 systems.
On a pure RV32 system (or when running RV32 software), 64-bit accesses
are impossible by definition. Therefore, if the spec were interpreted
as making 64-bit access mandatory, the statement itself would become
self-contradictory on RV32 — which is clearly not the intent.

The specification is deliberately precise. It uses "may", not "shall"
or "must". We should not introduce speculative implications that break
the spec's consistency on RV32. The safe and portable interpretation
is that software is explicitly allowed to use 32-bit accesses for
these registers on any system, which is exactly what this patch does.

> 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...
>
> Thanks,
> Robin.
>
> >
> >   #define riscv_iommu_writel(iommu, addr, val) \
> >       writel_relaxed((val), (iommu)->reg + (addr))
> >
> >   #define riscv_iommu_writeq(iommu, addr, val) \
> > -     writeq_relaxed((val), (iommu)->reg + (addr))
> > -
> > -#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> > -     readx_poll_timeout(readq_relaxed, (iommu)->reg + (addr), val, cond, \
> > -                        delay_us, timeout_us)
> > +     hi_lo_writeq_relaxed((val), (iommu)->reg + (addr))
> >
> >   #define riscv_iommu_readl_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> >       readx_poll_timeout(readl_relaxed, (iommu)->reg + (addr), val, cond, \
>


-- 
Best Regards
 Guo Ren

On Tue, Jul 14, 2026 at 09:55:19PM +0800, Guo Ren wrote:
> The specification is deliberately precise. It uses "may", not "shall"
> or "must". We should not introduce speculative implications that break
> the spec's consistency on RV32. The safe and portable interpretation
> is that software is explicitly allowed to use 32-bit accesses for
> these registers on any system, which is exactly what this patch does.

The RV32 argument is quite compelling. The RISC-V IOMMU is a
platform-level IP that must serve both RV32 and RV64 software stacks.
Mandating 64-bit MMIO accesses would directly conflict with RV32, where
such accesses are impossible — so any interpretation that turns "may be
accessed using either a 32-bit or a 64-bit access" into an implicit
hardware requirement for 64-bit accesses breaks the spec's own
consistency on RV32.

The RFC 2119 argument is equally strong. In RFC 2119 terms, "MAY" means
an item is truly optional — a specification should express obligations
through these well-defined normative keywords, not leave them to be
inferred from everyday-language intuition. If the spec authors had
intended to require hardware to support 64-bit accesses, they would have
said "must" or "shall".

Given both points, treating 32-bit MMIO accesses as the default for the
RISC-V IOMMU driver is the safe, portable, and more inclusive choice: it
is explicitly permitted on every conformant implementation, whereas
64-bit access atomicity is left unspecified.

Acked-by: Chen Pei <cp0613@linux.alibaba.com>

Thanks,
Pei

On Wed, Jul 15, 2026 at 5:43 PM Chen Pei <cp0613@linux.alibaba.com> wrote:
>
> On Tue, Jul 14, 2026 at 09:55:19PM +0800, Guo Ren wrote:
> > The specification is deliberately precise. It uses "may", not "shall"
> > or "must". We should not introduce speculative implications that break
> > the spec's consistency on RV32. The safe and portable interpretation
> > is that software is explicitly allowed to use 32-bit accesses for
> > these registers on any system, which is exactly what this patch does.
>
> The RV32 argument is quite compelling. The RISC-V IOMMU is a
> platform-level IP that must serve both RV32 and RV64 software stacks.
> Mandating 64-bit MMIO accesses would directly conflict with RV32, where
> such accesses are impossible — so any interpretation that turns "may be
> accessed using either a 32-bit or a 64-bit access" into an implicit
> hardware requirement for 64-bit accesses breaks the spec's own
> consistency on RV32.
>
> The RFC 2119 argument is equally strong. In RFC 2119 terms, "MAY" means
> an item is truly optional — a specification should express obligations
> through these well-defined normative keywords, not leave them to be
> inferred from everyday-language intuition. If the spec authors had
> intended to require hardware to support 64-bit accesses, they would have
> said "must" or "shall".
>
> Given both points, treating 32-bit MMIO accesses as the default for the
> RISC-V IOMMU driver is the safe, portable, and more inclusive choice: it
> is explicitly permitted on every conformant implementation, whereas
> 64-bit access atomicity is left unspecified.
>
> Acked-by: Chen Pei <cp0613@linux.alibaba.com>
>

It seems that we might not need to worry about RV32. The
io-64-nonatomic-hi-lo.h header will automatically replace writeq/readq
with hi_lo_writeq/readq in RV32 build. Therefore, perhaps we can just
focus our discussion on RV64: whether the driver should use
readq/writeq to access 64-bit registers on RV64 systems.

Actually, in my previous IOMMU PMU series, I shared a similar view to Robin's:
"Revisiting the spec regarding the UNSPECIFIED 8-byte atomic access,
this might imply that the hardware itself will handle the split into
two 4-byte accesses and internally manage its own trigger logic.
Because of this, for RV64 systems, software might simply use standard
8-byte writes (e.g., writeq). Delegating the access handling to the
hardware is likely the best way to maximize compatibility across all
vendor implementations."
More details: https://lists.infradead.org/pipermail/linux-riscv/2026-July/094099.html

I also checked the GitHub discussions. The IOMMU spec author seems to
have clarified that the hardware should be able to handle naturally
aligned 64-bit accesses from the software.
More details: https://github.com/riscv-non-isa/riscv-iommu/issues/765

From my perspective, since the author has already clarified the
intent, it might be better for us to focus on improving and updating
the spec to make it clearer. This could be a better approach than
changing the driver to fit an unclear spec.
What does everyone think about this? If we all agree, I would be happy
to send a new version of the IOMMU PMU series using 64-bit accesses
again for RV64.

> Thanks,
> Pei
>

On Thu, Jul 16, 2026 at 11:13 AM Zong Li <zong.li@sifive.com> wrote:
>
> On Wed, Jul 15, 2026 at 5:43 PM Chen Pei <cp0613@linux.alibaba.com> wrote:
> >
> > On Tue, Jul 14, 2026 at 09:55:19PM +0800, Guo Ren wrote:
> > > The specification is deliberately precise. It uses "may", not "shall"
> > > or "must". We should not introduce speculative implications that break
> > > the spec's consistency on RV32. The safe and portable interpretation
> > > is that software is explicitly allowed to use 32-bit accesses for
> > > these registers on any system, which is exactly what this patch does.
> >
> > The RV32 argument is quite compelling. The RISC-V IOMMU is a
> > platform-level IP that must serve both RV32 and RV64 software stacks.
> > Mandating 64-bit MMIO accesses would directly conflict with RV32, where
> > such accesses are impossible — so any interpretation that turns "may be
> > accessed using either a 32-bit or a 64-bit access" into an implicit
> > hardware requirement for 64-bit accesses breaks the spec's own
> > consistency on RV32.
> >
> > The RFC 2119 argument is equally strong. In RFC 2119 terms, "MAY" means
> > an item is truly optional — a specification should express obligations
> > through these well-defined normative keywords, not leave them to be
> > inferred from everyday-language intuition. If the spec authors had
> > intended to require hardware to support 64-bit accesses, they would have
> > said "must" or "shall".
> >
> > Given both points, treating 32-bit MMIO accesses as the default for the
> > RISC-V IOMMU driver is the safe, portable, and more inclusive choice: it
> > is explicitly permitted on every conformant implementation, whereas
> > 64-bit access atomicity is left unspecified.
> >
> > Acked-by: Chen Pei <cp0613@linux.alibaba.com>
> >
>
> It seems that we might not need to worry about RV32. The
> io-64-nonatomic-hi-lo.h header will automatically replace writeq/readq
> with hi_lo_writeq/readq in RV32 build. Therefore, perhaps we can just
> focus our discussion on RV64: whether the driver should use
> readq/writeq to access 64-bit registers on RV64 systems.

The RV32 argument here concerns the interpretation of the
specification, not the Linux implementation. Some people interpret the
statement, “Registers that are 64-bit wide may be accessed using
either a 32-bit or a 64-bit access,” as imposing an obligation on
every hardware implementation to support native 64-bit MMIO accesses.
However, RV32 provides a direct counterexample that undermines the
validity of this interpretation. The IOMMU is a platform-level
component that may be controlled either by an RV32 MCU running an RTOS
or by an RV64 processor running Linux. On an RV32 platform, software
can access a 64-bit register only through multiple 32-bit
transactions. How, then, can this sentence be interpreted as
unconditionally requiring the hardware to support native 64-bit MMIO
accesses?

Moreover, even if “may” is interpreted in the RFC 2119 sense, MAY does
not impose a mandatory requirement. We should therefore avoid
inferring hardware obligations from assumptions or implications that
are not explicitly stated in the specification.

>
> Actually, in my previous IOMMU PMU series, I shared a similar view to Robin's:
> "Revisiting the spec regarding the UNSPECIFIED 8-byte atomic access,
> this might imply that the hardware itself will handle the split into
> two 4-byte accesses and internally manage its own trigger logic.
> Because of this, for RV64 systems, software might simply use standard
> 8-byte writes (e.g., writeq). Delegating the access handling to the
> hardware is likely the best way to maximize compatibility across all
> vendor implementations."
> More details: https://lists.infradead.org/pipermail/linux-riscv/2026-July/094099.html
>
> I also checked the GitHub discussions. The IOMMU spec author seems to
> have clarified that the hardware should be able to handle naturally
> aligned 64-bit accesses from the software.
> More details: https://github.com/riscv-non-isa/riscv-iommu/issues/765
>
> From my perspective, since the author has already clarified the
> intent, it might be better for us to focus on improving and updating
> the spec to make it clearer. This could be a better approach than
> changing the driver to fit an unclear spec.
> What does everyone think about this? If we all agree, I would be happy
> to send a new version of the IOMMU PMU series using 64-bit accesses
> again for RV64.

This issue has already become highly contentious and directly affects
real hardware implementations. Any attempt to revise the specification
must first clarify whether the current wording actually mandates
support for 64-bit MMIO accesses. If the specification is unclear on
this point, then no such mandatory requirement can reasonably be
imposed on existing hardware implementations.

--
Best Regards
 Guo Ren

On Thu, Jul 16, 2026 at 12:04 PM Guo Ren <guoren@kernel.org> wrote:
>
> On Thu, Jul 16, 2026 at 11:13 AM Zong Li <zong.li@sifive.com> wrote:
> >
> > On Wed, Jul 15, 2026 at 5:43 PM Chen Pei <cp0613@linux.alibaba.com> wrote:
> > >
> > > On Tue, Jul 14, 2026 at 09:55:19PM +0800, Guo Ren wrote:
> > > > The specification is deliberately precise. It uses "may", not "shall"
> > > > or "must". We should not introduce speculative implications that break
> > > > the spec's consistency on RV32. The safe and portable interpretation
> > > > is that software is explicitly allowed to use 32-bit accesses for
> > > > these registers on any system, which is exactly what this patch does.
> > >
> > > The RV32 argument is quite compelling. The RISC-V IOMMU is a
> > > platform-level IP that must serve both RV32 and RV64 software stacks.
> > > Mandating 64-bit MMIO accesses would directly conflict with RV32, where
> > > such accesses are impossible — so any interpretation that turns "may be
> > > accessed using either a 32-bit or a 64-bit access" into an implicit
> > > hardware requirement for 64-bit accesses breaks the spec's own
> > > consistency on RV32.
> > >
> > > The RFC 2119 argument is equally strong. In RFC 2119 terms, "MAY" means
> > > an item is truly optional — a specification should express obligations
> > > through these well-defined normative keywords, not leave them to be
> > > inferred from everyday-language intuition. If the spec authors had
> > > intended to require hardware to support 64-bit accesses, they would have
> > > said "must" or "shall".
> > >
> > > Given both points, treating 32-bit MMIO accesses as the default for the
> > > RISC-V IOMMU driver is the safe, portable, and more inclusive choice: it
> > > is explicitly permitted on every conformant implementation, whereas
> > > 64-bit access atomicity is left unspecified.
> > >
> > > Acked-by: Chen Pei <cp0613@linux.alibaba.com>
> > >
> >
> > It seems that we might not need to worry about RV32. The
> > io-64-nonatomic-hi-lo.h header will automatically replace writeq/readq
> > with hi_lo_writeq/readq in RV32 build. Therefore, perhaps we can just
> > focus our discussion on RV64: whether the driver should use
> > readq/writeq to access 64-bit registers on RV64 systems.
>
> The RV32 argument here concerns the interpretation of the
> specification, not the Linux implementation. Some people interpret the

Yes. However, since these patches are indeed trying to modify the
Linux implementation, we might not need to worry about this here. On
RV32, regardless of whether the IOMMU hardware supports native 64-bit
MMIO accesses, Linux automatically uses two 4-byte accesses for 64-bit
registers.

> statement, “Registers that are 64-bit wide may be accessed using
> either a 32-bit or a 64-bit access,” as imposing an obligation on
> every hardware implementation to support native 64-bit MMIO accesses.
> However, RV32 provides a direct counterexample that undermines the
> validity of this interpretation. The IOMMU is a platform-level
> component that may be controlled either by an RV32 MCU running an RTOS
> or by an RV64 processor running Linux. On an RV32 platform, software
> can access a 64-bit register only through multiple 32-bit
> transactions. How, then, can this sentence be interpreted as
> unconditionally requiring the hardware to support native 64-bit MMIO
> accesses?
>
> Moreover, even if “may” is interpreted in the RFC 2119 sense, MAY does
> not impose a mandatory requirement. We should therefore avoid
> inferring hardware obligations from assumptions or implications that
> are not explicitly stated in the specification.
>
> >
> > Actually, in my previous IOMMU PMU series, I shared a similar view to Robin's:
> > "Revisiting the spec regarding the UNSPECIFIED 8-byte atomic access,
> > this might imply that the hardware itself will handle the split into
> > two 4-byte accesses and internally manage its own trigger logic.
> > Because of this, for RV64 systems, software might simply use standard
> > 8-byte writes (e.g., writeq). Delegating the access handling to the
> > hardware is likely the best way to maximize compatibility across all
> > vendor implementations."
> > More details: https://lists.infradead.org/pipermail/linux-riscv/2026-July/094099.html
> >
> > I also checked the GitHub discussions. The IOMMU spec author seems to
> > have clarified that the hardware should be able to handle naturally
> > aligned 64-bit accesses from the software.
> > More details: https://github.com/riscv-non-isa/riscv-iommu/issues/765
> >
> > From my perspective, since the author has already clarified the
> > intent, it might be better for us to focus on improving and updating
> > the spec to make it clearer. This could be a better approach than
> > changing the driver to fit an unclear spec.
> > What does everyone think about this? If we all agree, I would be happy
> > to send a new version of the IOMMU PMU series using 64-bit accesses
> > again for RV64.
>
> This issue has already become highly contentious and directly affects
> real hardware implementations. Any attempt to revise the specification
> must first clarify whether the current wording actually mandates
> support for 64-bit MMIO accesses. If the specification is unclear on
> this point, then no such mandatory requirement can reasonably be
> imposed on existing hardware implementations.
>

Yes, I agree. That is why I think we might focus on making the spec
clearer before changing the driver. Would it make sense to continue
the discussion on IOMMU spec GitHub? Perhaps reopen the same issue or
create a new one.

> --
> Best Regards
>  Guo Ren

On Thu, Jul 16, 2026 at 2:59 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Thu, Jul 16, 2026 at 12:04 PM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Thu, Jul 16, 2026 at 11:13 AM Zong Li <zong.li@sifive.com> wrote:
> > >
> > > On Wed, Jul 15, 2026 at 5:43 PM Chen Pei <cp0613@linux.alibaba.com> wrote:
> > > >
> > > > On Tue, Jul 14, 2026 at 09:55:19PM +0800, Guo Ren wrote:
> > > > > The specification is deliberately precise. It uses "may", not "shall"
> > > > > or "must". We should not introduce speculative implications that break
> > > > > the spec's consistency on RV32. The safe and portable interpretation
> > > > > is that software is explicitly allowed to use 32-bit accesses for
> > > > > these registers on any system, which is exactly what this patch does.
> > > >
> > > > The RV32 argument is quite compelling. The RISC-V IOMMU is a
> > > > platform-level IP that must serve both RV32 and RV64 software stacks.
> > > > Mandating 64-bit MMIO accesses would directly conflict with RV32, where
> > > > such accesses are impossible — so any interpretation that turns "may be
> > > > accessed using either a 32-bit or a 64-bit access" into an implicit
> > > > hardware requirement for 64-bit accesses breaks the spec's own
> > > > consistency on RV32.
> > > >
> > > > The RFC 2119 argument is equally strong. In RFC 2119 terms, "MAY" means
> > > > an item is truly optional — a specification should express obligations
> > > > through these well-defined normative keywords, not leave them to be
> > > > inferred from everyday-language intuition. If the spec authors had
> > > > intended to require hardware to support 64-bit accesses, they would have
> > > > said "must" or "shall".
> > > >
> > > > Given both points, treating 32-bit MMIO accesses as the default for the
> > > > RISC-V IOMMU driver is the safe, portable, and more inclusive choice: it
> > > > is explicitly permitted on every conformant implementation, whereas
> > > > 64-bit access atomicity is left unspecified.
> > > >
> > > > Acked-by: Chen Pei <cp0613@linux.alibaba.com>
> > > >
> > >
> > > It seems that we might not need to worry about RV32. The
> > > io-64-nonatomic-hi-lo.h header will automatically replace writeq/readq
> > > with hi_lo_writeq/readq in RV32 build. Therefore, perhaps we can just
> > > focus our discussion on RV64: whether the driver should use
> > > readq/writeq to access 64-bit registers on RV64 systems.
> >
> > The RV32 argument here concerns the interpretation of the
> > specification, not the Linux implementation. Some people interpret the
>
> Yes. However, since these patches are indeed trying to modify the
> Linux implementation, we might not need to worry about this here. On
> RV32, regardless of whether the IOMMU hardware supports native 64-bit
> MMIO accesses, Linux automatically uses two 4-byte accesses for 64-bit
> registers.

The RV32 argument highlights a logical inconsistency in interpreting
“Registers that are 64-bit wide may be accessed using either a 32-bit
or a 64-bit access” as implying that hardware must support native
64-bit MMIO accesses.

Please note that an IOMMU is a platform-level device and is not
inherently tied to either RV32 or RV64. It may be accessed by any bus
master in the system. RV32 is cited here not to discuss how Linux
implements these accesses, but to demonstrate that an unconditional
requirement for the hardware to support native 64-bit MMIO accesses
would be logically inconsistent.


>
> > statement, “Registers that are 64-bit wide may be accessed using
> > either a 32-bit or a 64-bit access,” as imposing an obligation on
> > every hardware implementation to support native 64-bit MMIO accesses.
> > However, RV32 provides a direct counterexample that undermines the
> > validity of this interpretation. The IOMMU is a platform-level
> > component that may be controlled either by an RV32 MCU running an RTOS
> > or by an RV64 processor running Linux. On an RV32 platform, software
> > can access a 64-bit register only through multiple 32-bit
> > transactions. How, then, can this sentence be interpreted as
> > unconditionally requiring the hardware to support native 64-bit MMIO
> > accesses?
> >
> > Moreover, even if “may” is interpreted in the RFC 2119 sense, MAY does
> > not impose a mandatory requirement. We should therefore avoid
> > inferring hardware obligations from assumptions or implications that
> > are not explicitly stated in the specification.
> >
> > >
> > > Actually, in my previous IOMMU PMU series, I shared a similar view to Robin's:
> > > "Revisiting the spec regarding the UNSPECIFIED 8-byte atomic access,
> > > this might imply that the hardware itself will handle the split into
> > > two 4-byte accesses and internally manage its own trigger logic.
> > > Because of this, for RV64 systems, software might simply use standard
> > > 8-byte writes (e.g., writeq). Delegating the access handling to the
> > > hardware is likely the best way to maximize compatibility across all
> > > vendor implementations."
> > > More details: https://lists.infradead.org/pipermail/linux-riscv/2026-July/094099.html
> > >
> > > I also checked the GitHub discussions. The IOMMU spec author seems to
> > > have clarified that the hardware should be able to handle naturally
> > > aligned 64-bit accesses from the software.
> > > More details: https://github.com/riscv-non-isa/riscv-iommu/issues/765
> > >
> > > From my perspective, since the author has already clarified the
> > > intent, it might be better for us to focus on improving and updating
> > > the spec to make it clearer. This could be a better approach than
> > > changing the driver to fit an unclear spec.
> > > What does everyone think about this? If we all agree, I would be happy
> > > to send a new version of the IOMMU PMU series using 64-bit accesses
> > > again for RV64.
> >
> > This issue has already become highly contentious and directly affects
> > real hardware implementations. Any attempt to revise the specification
> > must first clarify whether the current wording actually mandates
> > support for 64-bit MMIO accesses. If the specification is unclear on
> > this point, then no such mandatory requirement can reasonably be
> > imposed on existing hardware implementations.
> >
>
> Yes, I agree. That is why I think we might focus on making the spec
> clearer before changing the driver. Would it make sense to continue
> the discussion on IOMMU spec GitHub? Perhaps reopen the same issue or
> create a new one.

I agree that the issue should be reopened. However, I do not think it
should necessarily block this patch, since it makes the driver use the
32-bit MMIO access path explicitly supported by the current
specification. The correctness of the patch can be reviewed based on
the required access ordering and register-specific semantics,
independently of the broader discussion about mandatory support for
64-bit MMIO transactions.

If the current specification does not clearly establish a mandatory
requirement for 64-bit MMIO accesses, any future clarification should
avoid retroactively invalidating existing implementations that were
built according to a reasonable reading of the current text.

If support for 64-bit MMIO transactions is intended to be optional,
that capability could be explicitly defined, along with an appropriate
mechanism for software to discover it.

Making such support unconditionally mandatory through a later
specification change would require careful consideration of the
compatibility impact on existing hardware.

--
Best Regards
 Guo Ren

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.

-- 
Best Regards
 Guo Ren

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.

	David

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.

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.

-- 
Best Regards
 Guo Ren

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.
> 

On Wed, Jul 15, 2026 at 5:02 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> 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 RV IOMMU specification does not restrict the order of software
accesses, and high-to-low ordering is for hardware 64-bit MMIO
accesses; the spec does not mention any latch requirement. Therefore,
we cannot assert that this ordering exists to support latching — this
remains speculation. Although I consider the speculation plausible, it
cannot be regarded as a mandatory requirement imposed by the
specification on hardware; these are two separate matters.

The specification is ambiguous about the atomicity of 64-bit MMIO
access and explicitly states that such atomicity is UNSPECIFIC. This
ambiguity likely arises from the hardware latching behavior associated
with atomicity (as you mentioned). Consequently, leaving it as
UNSPECIFIC does not encourage hardware implementations to provide
atomic 64-bit MMIO access. If 64-bit MMIO access does not guarantee
atomicity, then what advantage does it actually offer?


>
> 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.
>

According to RFC 2119, the term “may” carries no mandatory force. It
is incorrect to interpret normative language in the specification
through everyday intuition rather than its formal meaning. The
specification does not explicitly require that RV64 software "must"
perform 64-bit MMIO accesses to 64-bit registers. In the absence of
such a direct requirement, no obligation can be inferred.

Since the IOMMU is a bus-attached device that must remain accessible
to RV32 systems, mandating 64-bit accesses for RV32 would be
self-contradictory.
A clear logical consequence follows directly from the specification:
Because the RISC-V IOMMU explicitly supports RV32 software, a pure
32-bit MMIO access model is valid and must be accommodated.

-- 
Best Regards
 Guo Ren

LGTM!

Reviewed-by: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>
Co-developed-by: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>
Signed-off-by: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>

On Mon, Jul 13, 2026 at 8:29 PM Zhanpeng Zhang
<zhangzhanpeng.jasper@bytedance.com> 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); \
> +       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))
>
>  #define riscv_iommu_writel(iommu, addr, val) \
>         writel_relaxed((val), (iommu)->reg + (addr))
>
>  #define riscv_iommu_writeq(iommu, addr, val) \
> -       writeq_relaxed((val), (iommu)->reg + (addr))
> -
> -#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> -       readx_poll_timeout(readq_relaxed, (iommu)->reg + (addr), val, cond, \
> -                          delay_us, timeout_us)
> +       hi_lo_writeq_relaxed((val), (iommu)->reg + (addr))
>
>  #define riscv_iommu_readl_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
>         readx_poll_timeout(readl_relaxed, (iommu)->reg + (addr), val, cond, \
> --
> 2.50.1 (Apple Git-155)

-- 
Best Regards
 Guo Ren

On 7/13/26 7:02 PM, Guo Ren wrote:
> LGTM!
>
> Reviewed-by: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>
> Co-developed-by: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>
> Signed-off-by: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>
>
> On Mon, Jul 13, 2026 at 8:29 PM Zhanpeng Zhang
> <zhangzhanpeng.jasper@bytedance.com> 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); \
>> +       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))
>>
>>   #define riscv_iommu_writel(iommu, addr, val) \
>>          writel_relaxed((val), (iommu)->reg + (addr))
>>
>>   #define riscv_iommu_writeq(iommu, addr, val) \
>> -       writeq_relaxed((val), (iommu)->reg + (addr))
>> -
>> -#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
>> -       readx_poll_timeout(readq_relaxed, (iommu)->reg + (addr), val, cond, \
>> -                          delay_us, timeout_us)
>> +       hi_lo_writeq_relaxed((val), (iommu)->reg + (addr))
>>
>>   #define riscv_iommu_readl_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
>>          readx_poll_timeout(readl_relaxed, (iommu)->reg + (addr), val, cond, \
>> --
>> 2.50.1 (Apple Git-155)

Thank you for this change. Sorry for being late on the discussion. LGTM

Reviewed-by: Tomasz Jeznach <tomasz.jeznach@linux.dev>

Best,
- Tomasz