Re: [PATCH v1] iommu/riscv: Support 32-bit register accesses

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

On Mon, 15 Jun 2026 14:48:55 +0800
Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com> 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@bytedance.com>
> Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
> Signed-off-by: Zhanpeng Zhang <zhangzhanpeng.jasper@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);

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