Re: [External] Re: [PATCH v1] iommu/riscv: Support 32-bit register accesses
From: Zhanpeng Zhang
Date: Mon Jun 15 2026 - 09:25:34 EST
Hi David,
Thanks, I agree with your point about the lock and the relaxed accessors.
Guo Ren also pointed me to the earlier discussion on this topic, and I think the
right next step is to align with that thread rather than continue this separate
Kconfig-based patch.
So I will drop this patch.
Thanks,
Zhanpeng
> From: "David Laight"<david.laight.linux@xxxxxxxxx>
> Date: Mon, Jun 15, 2026, 18:00
> Subject: [External] Re: [PATCH v1] iommu/riscv: Support 32-bit register accesses
> To: "Zhanpeng Zhang"<zhangzhanpeng.jasper@xxxxxxxxxxxxx>
> Cc: "Tomasz Jeznach"<tjeznach@xxxxxxxxxxxx>, "Joerg Roedel"<joro@xxxxxxxxxx>, "Will Deacon"<will@xxxxxxxxxx>, "Robin Murphy"<robin.murphy@xxxxxxx>, "Paul Walmsley"<pjw@xxxxxxxxxx>, "Palmer Dabbelt"<palmer@xxxxxxxxxxx>, "Albert Ou"<aou@xxxxxxxxxxxxxxxxx>, "Alexandre Ghiti"<alex@xxxxxxxx>, <iommu@xxxxxxxxxxxxxxx>, <linux-riscv@xxxxxxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, "Xu Lu"<luxu.kernel@xxxxxxxxxxxxx>, <cuiyunhui@xxxxxxxxxxxxx>, <yuanzhu@xxxxxxxxxxxxx>
> On Mon, 15 Jun 2026 14:48:55 +0800
> Zhanpeng Zhang <zhangzhanpeng.jasper@xxxxxxxxxxxxx> wrote:
>
> > Some RISC-V IOMMU implementations cannot perform 64-bit MMIO accesses
> > to the IOMMU register file. The RISC-V IOMMU architecture allows 64-bit
> > registers to be accessed using 32-bit accesses, provided the accesses are
> > properly aligned and do not span multiple registers.
> >
> > Add a config option for such implementations and access 64-bit IOMMU
> > registers as paired 32-bit MMIO operations when it is enabled. Serialize
> > the paired accesses so the high and low halves cannot interleave with
> > another CPU. Full 64-bit register programming writes the high half before
> > the low half.
> >
> > This option describes the register access width. It is not an RV32 kernel
> > mode and does not describe a 32-bit IOMMU architecture.
> >
> > Co-developed-by: Xu Lu <luxu.kernel@xxxxxxxxxxxxx>
> > Signed-off-by: Xu Lu <luxu.kernel@xxxxxxxxxxxxx>
> > Signed-off-by: Zhanpeng Zhang <zhangzhanpeng.jasper@xxxxxxxxxxxxx>
> > ---
> > This is needed for platforms whose RISC-V IOMMU register window does not
> > support naturally aligned 64-bit MMIO accesses.
> >
> > drivers/iommu/riscv/Kconfig | 11 ++++++++
> > drivers/iommu/riscv/iommu.c | 4 +++
> > drivers/iommu/riscv/iommu.h | 55 +++++++++++++++++++++++++++++++++----
> > 3 files changed, 64 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/riscv/Kconfig b/drivers/iommu/riscv/Kconfig
> > index b86e5ab94183..54d624b9b2ef 100644
> > --- a/drivers/iommu/riscv/Kconfig
> > +++ b/drivers/iommu/riscv/Kconfig
> > @@ -22,3 +22,14 @@ config RISCV_IOMMU_PCI
> > def_bool y if RISCV_IOMMU && PCI_MSI
> > help
> > Support for the PCIe implementation of RISC-V IOMMU architecture.
> > +
> > +config RISCV_IOMMU_32BIT_ACCESS
> > + bool "Use 32-bit accesses for RISC-V IOMMU registers"
> > + depends on RISCV_IOMMU
> > + help
> > + Say Y when the RISC-V IOMMU MMIO window cannot be accessed
> > + using naturally aligned 64-bit loads and stores.
> > +
> > + When enabled, 64-bit IOMMU registers are accessed as paired
> > + 32-bit MMIO operations. This option does not describe an RV32
> > + kernel or a 32-bit IOMMU architecture.
> > diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> > index a31f50bbad35..7fa1721b5728 100644
> > --- a/drivers/iommu/riscv/iommu.c
> > +++ b/drivers/iommu/riscv/iommu.c
> > @@ -53,6 +53,10 @@ struct riscv_iommu_devres {
> > void *addr;
> > };
> >
> > +#ifdef CONFIG_RISCV_IOMMU_32BIT_ACCESS
> > +DEFINE_RAW_SPINLOCK(riscv_iommu_32bit_access_lock);
> > +#endif
> > +
> > static void riscv_iommu_devres_pages_release(struct device *dev, void *res)
> > {
> > struct riscv_iommu_devres *devres = res;
> > diff --git a/drivers/iommu/riscv/iommu.h b/drivers/iommu/riscv/iommu.h
> > index 46df79dd5495..ba78ef1858c5 100644
> > --- a/drivers/iommu/riscv/iommu.h
> > +++ b/drivers/iommu/riscv/iommu.h
> > @@ -14,6 +14,9 @@
> > #include <linux/iommu.h>
> > #include <linux/types.h>
> > #include <linux/iopoll.h>
> > +#ifdef CONFIG_RISCV_IOMMU_32BIT_ACCESS
> > +#include <linux/spinlock.h>
> > +#endif
> >
> > #include "iommu-bits.h"
> >
> > @@ -69,21 +72,61 @@ void riscv_iommu_disable(struct riscv_iommu_device *iommu);
> > #define riscv_iommu_readl(iommu, addr) \
> > readl_relaxed((iommu)->reg + (addr))
> >
> > -#define riscv_iommu_readq(iommu, addr) \
> > - readq_relaxed((iommu)->reg + (addr))
> > -
> > #define riscv_iommu_writel(iommu, addr, val) \
> > writel_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, \
> > + delay_us, timeout_us)
> > +
> > +#ifndef CONFIG_RISCV_IOMMU_32BIT_ACCESS
> > +#define riscv_iommu_readq(iommu, addr) \
> > + readq_relaxed((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)
> > +#else /* CONFIG_RISCV_IOMMU_32BIT_ACCESS */
> >
> > -#define riscv_iommu_readl_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> > - readx_poll_timeout(readl_relaxed, (iommu)->reg + (addr), val, cond, \
> > - delay_us, timeout_us)
> > +extern raw_spinlock_t riscv_iommu_32bit_access_lock;
> > +
> > +static inline u64 __riscv_iommu_readq_relaxed(void __iomem *addr)
> > +{
> > + u32 lo, hi;
> > + unsigned long flags;
> > +
> > + raw_spin_lock_irqsave(&riscv_iommu_32bit_access_lock, flags);
> > + do {
> > + hi = readl_relaxed(addr + sizeof(u32));
> > + lo = readl_relaxed(addr);
> > + } while (hi != readl_relaxed(addr + sizeof(u32)));
> > + raw_spin_unlock_irqrestore(&riscv_iommu_32bit_access_lock, flags);
>
> Why the lock and the loop?
> If you need the loop (for places where the hardware might be changing
> both words?) then there is no need to read 'hi' twice when you loop.
>
> But does it make any sense to be reading the value either while the hardware
> is updating it (does that happen?) or concurrently with a write.
>
>
> > +
> > + return ((u64)hi << 32) | (u64)lo;
> > +}
> > +
> > +static inline void __riscv_iommu_writeq_relaxed(u64 value, void __iomem *addr)
> > +{
> > + unsigned long flags;
> > +
> > + raw_spin_lock_irqsave(&riscv_iommu_32bit_access_lock, flags);
> > + writel_relaxed((u32)(value >> 32), addr + sizeof(u32));
> > + writel_relaxed((u32)value, addr);
> > + raw_spin_unlock_irqrestore(&riscv_iommu_32bit_access_lock, flags);
>
> What are you actually locking against?
> I'm pretty sure it doesn't make any sense for two cpu to be writing to the
> same entry at the same time.
>
> Can't the writel_relaxed() be re-ordered, so you aren't guaranteeing they'll
> be done high-low.
> Same with the reads.
>
> David
>
> > +}
> > +
> > +#define riscv_iommu_readq(iommu, addr) \
> > + __riscv_iommu_readq_relaxed((iommu)->reg + (addr))
> > +
> > +#define riscv_iommu_writeq(iommu, addr, val) \
> > + __riscv_iommu_writeq_relaxed((val), (iommu)->reg + (addr))
> > +
> > +#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> > + readx_poll_timeout(__riscv_iommu_readq_relaxed, (iommu)->reg + (addr), \
> > + val, cond, delay_us, timeout_us)
> > +#endif /* CONFIG_RISCV_IOMMU_32BIT_ACCESS */
> >
> > #endif
>