[External] Re: [PATCH v1] iommu/riscv: Support 32-bit register accesses
Zhanpeng Zhang
zhangzhanpeng.jasper at bytedance.com
Mon Jun 15 06:23:07 PDT 2026
Hi Guo Ren,
Thanks for the pointer, and sorry that I missed the earlier discussion.
I agree with your concerns. The lock in my v1 is not the right approach, and
David also pointed out the same issue. More importantly, after reading your
comments, I agree that this should not be handled as a separate Kconfig-selected
path.
If the RISC-V IOMMU specification allows 64-bit registers to be accessed with
32-bit MMIO operations, then a generic kernel should be able to support that
without requiring a platform-specific build-time option. So I will drop my v1
and will not continue this separate patch.
I will take a closer look at your earlier proposal and the previous discussion.
If the preferred direction is to make 32-bit register accesses part of the
driver's common register access model, I am happy to help test, review, or
rework patches in that direction.
Best Regards,
Zhanpeng
> From: "Guo Ren"<guoren at kernel.org>
> Date: Mon, Jun 15, 2026, 20:38
> Subject: [External] Re: [PATCH v1] iommu/riscv: Support 32-bit register accesses
> To: <zhangzhanpeng.jasper at bytedance.com>
> Cc: <alex at ghiti.fr>, <aou at eecs.berkeley.edu>, <cuiyunhui at bytedance.com>, <iommu at lists.linux.dev>, <joro at 8bytes.org>, <linux-kernel at vger.kernel.org>, <linux-riscv at lists.infradead.org>, <luxu.kernel at bytedance.com>, <palmer at dabbelt.com>, <pjw at kernel.org>, <robin.murphy at arm.com>, <tjeznach at rivosinc.com>, <will at kernel.org>, <yuanzhu at bytedance.com>
> Hi Zhanpeng Zhang,
>
> I noticed you posted a similar fix recently. However, I had already
> submitted a similar solution back in September 2025 [1]. It would be
> great if you could review it. A few concerns with the current patch:
>
> 1. Using spin_lock_irqsave in such a small semantic structure is
> unnecessary. IRQ protection should be provided by the higher-level
> IOMMU driver interfaces. Many existing call sites already handle IRQ
> disabling, so adding it here feels redundant and adds avoidable
> overhead.
>
> 2. More critically, the RISCV_IOMMU_32BIT_ACCESS Kconfig option is
> problematic. According to the RISC-V IOMMU specification (Software
> Guidelines, “Reading and writing IOMMU registers”):
>
> > Registers that are 64-bit wide may be accessed using either a 32-bit
> > or a 64-bit access.
>
> This clearly requires that any hardware supporting 64-bit MMIO access
> must also support 32-bit MMIO access. Therefore, the IOMMU driver should
> be built on a 32-bit access base for maximum hardware compatibility.
>
> 3. Only performance-monitoring counters require 64-bit IO access or the
> high-low-high do-while retry strategy. For ordinary status and control
> MMIO registers, a single read is sufficient.
>
> 4. Introducing a compile-time Kconfig that forces 32-bit splitting (or
> 64-bit access) across the entire driver is unnecessary for the vast
> majority of registers and would prevent a single kernel Image (or .ko)
> from working across all compliant platforms.
>
> I believe the correct approach is to keep the driver on a 32-bit access
> foundation (as required by the spec) and apply special 64-bit or
> high-low-high handling only to performance-monitoring counters when
> justified by data. This preserves broad hardware support without
> unnecessary complexity.
>
> What do you think? I’m happy to discuss or collaborate on a cleaner
> solution based on the earlier patch [1].
>
> [1]: https://lore.kernel.org/linux-riscv/20250903144217.837448-1-guoren@kernel.org/
>
> Best Regards
> GUO Ren
>
> > 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 at bytedance.com>
> > Signed-off-by: Xu Lu <luxu.kernel at bytedance.com>
> > Signed-off-by: Zhanpeng Zhang <zhangzhanpeng.jasper at bytedance.com>
> > ---
> > 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);
> > +
> > + 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);
> > +}
> > +
> > +#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
> > --
> > 2.50.1 (Apple Git-155)
>
More information about the linux-riscv
mailing list