Message-ID: <CAPYmKFtAWNz7nCDDD7rjGeV2fRBgy5Kt6KR60pEyXAM=bb7r4g@mail.gmail.com>
Date: Wed, 26 Mar 2025 11:26:07 +0800
From: Xu Lu <luxu.kernel@...edance.com>
To: Jessica Clarke <jrtc27@...c27.com>
Cc: tjeznach@...osinc.com, joro@...tes.org, will@...nel.org, 
	robin.murphy@....com, alex@...ti.fr, lihangjing@...edance.com, 
	xieyongji@...edance.com, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Subject: Re: [External] Re: [PATCH] iommu: riscv: Split 8-byte accesses on 32
 bit I/O bus platform

Hi Jessica,

> Is such a platform conformant to the specification?

We have talked about this before [1]. I think the IOMMU spec does not
mandate the implementation of 8-byte access functionality. The related
sentences are listed below:

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

"Registers that are 64-bit wide may be accessed using either a 32-bit
or a 64-bit access."

> Either way, why is this a static build-time configuration choice rather than a dynamic run-time choice based on the FDT / ACPI tables / some other platform probing method?

I did not find available field in RIMT table to describe it. So maybe
adding a new config is faster.

[1] https://www.uwsg.indiana.edu/hypermail/linux/kernel/2408.3/03968.html

Best Regards,

Xu Lu

On Wed, Mar 26, 2025 at 2:51 AM Jessica Clarke <jrtc27@...c27.com> wrote:
>
> On 25 Mar 2025, at 14:42, Xu Lu <luxu.kernel@...edance.com> wrote:
> >
> > Introduce a new configuration CONFIG_RISCV_IOMMU_32BIT to enable
> > splitting 8-byte access into 4-byte transactions for hardware platform
> > whose I/O bus limits access to 4-byte transfers.
> >
> > Signed-off-by: Xu Lu <luxu.kernel@...edance.com>
>
> Is such a platform conformant to the specification? Either way, why is
> this a static build-time configuration choice rather than a dynamic
> run-time choice based on the FDT / ACPI tables / some other platform
> probing method?
>
> Jess
>
> > ---
> > drivers/iommu/riscv/Kconfig |  9 +++++++++
> > drivers/iommu/riscv/iommu.h | 28 +++++++++++++++++++++++-----
> > 2 files changed, 32 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/riscv/Kconfig b/drivers/iommu/riscv/Kconfig
> > index c071816f59a6..b7c9ea22d969 100644
> > --- a/drivers/iommu/riscv/Kconfig
> > +++ b/drivers/iommu/riscv/Kconfig
> > @@ -18,3 +18,12 @@ 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
> > + bool "Support 4-Byte Accesses on RISC-V IOMMU Registers"
> > + depends on RISCV_IOMMU
> > + default n
> > + help
> > +  Support hardware platform whose I/O bus limits access to 4-byte
> > +  transfers. When enabled, all accesses to IOMMU registers will be
> > +  split into 4-byte accesses.
> > diff --git a/drivers/iommu/riscv/iommu.h b/drivers/iommu/riscv/iommu.h
> > index 46df79dd5495..0e3552a8142d 100644
> > --- a/drivers/iommu/riscv/iommu.h
> > +++ b/drivers/iommu/riscv/iommu.h
> > @@ -14,6 +14,10 @@
> > #include <linux/iommu.h>
> > #include <linux/types.h>
> > #include <linux/iopoll.h>
> > +#ifdef CONFIG_RISCV_IOMMU_32BIT
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#endif
> >
> > #include "iommu-bits.h"
> >
> > @@ -69,21 +73,35 @@ 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
> > +#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 */
> > +#define riscv_iommu_readq(iommu, addr) \
> > + hi_lo_readq_relaxed((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, \
> > +#define riscv_iommu_writeq(iommu, addr, val) \
> > + ((addr == RISCV_IOMMU_REG_IOHPMCYCLES) ? \
> > + lo_hi_writeq_relaxed((val), (iommu)->reg + (addr)) : \
> > + hi_lo_writeq_relaxed((val), (iommu)->reg + (addr)))
> > +
> > +#define riscv_iommu_readq_timeout(iommu, addr, val, cond, delay_us, timeout_us) \
> > + readx_poll_timeout(hi_lo_readq_relaxed, (iommu)->reg + (addr), val, cond, \
> >   delay_us, timeout_us)
> > +#endif /* CONFIG_RISCV_IOMMU_32BIT */
> >
> > #endif
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Powered by blists - more mailing lists