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

On Fri, Aug 28, 2026 at 3:52 PM David Laight
<[email protected]> wrote:
>
> On Fri, 28 Aug 2026 10:52:08 +0800
> Zong Li <[email protected]> wrote:
>
> > On Fri, Jun 19, 2026 at 12:02 AM David Laight
> > <[email protected]> wrote:
> > >
> > > On Thu, 18 Jun 2026 17:51:34 +0800
> > > Guo Ren <[email protected]> wrote:
> > >
> > > > Hi Vivian,
> > > >
> > > > As noted in the RISC-V IOMMU Specification, Chapter 6:
> > > > > Whether an 8-byte access to an IOMMU register is single-copy atomic is UNSPECIFIED, and such an access may appear, internally to the IOMMU, as if two separate 4-byte accesses — first to the high half and second to the low half — were performed.
> > > >
> > > > Therefore, the atomicity of 64-bit MMIO accesses is UNSPECIFIED and
> > > > not clearly defined in the current ratified RISC-V IOMMU
> > > > specification. To handle this correctly, the Linux RISC-V IOMMU driver
> > > > should fall back to 32-bit MMIO accesses when reading 64-bit registers
> > > > (e.g., performance counters). The behavior of 32-bit MMIO accesses is
> > > > more precisely defined in the RISC-V IOMMU specification.
> > > >
> > > > Thus, many hardware vendors implement 32-bit MMIO (rather than 64-bit
> > > > MMIO) based on the current ratified RISC-V IOMMU specification, and
> > > > this driver does not appear to benefit from 64-bit MMIO access either.
> > > > Performance is fundamentally constrained by bus latency; assuming that
> > > > simply reducing the number of accesses will improve performance is an
> > > > oversimplification that ignores the underlying hardware
> > > > characteristics.
> > >
> > > If the bus latency is significant it is almost certainly worth using
> > > memory accesses to avoid re-reading the hi register.
> > >
> > > Something like this might work:
> > >
> > > static volatile u32 hi_prev, lo_prev;
> > >
> > >         u32 hi = read_reg_hi();
> > >         u32 lo = read_reg_lo();
> > >
> > >         if (lo <= lo_prev || hi != hi_prev) {
> > >                 u32 hi_tmp = read_reg_hi;
> > >                 if (hi_tmp != hi) {
> > >                         hi = hi_tmp;
> > >                         lo = 0;
> > >                 }
> > >                 lo_prev = ~0u;
> > >                 hi_prev = hi;
> > >         }
> > >         lo_prev = lo;
> > >         return (u64)hi << 32 | lo;
> > >
> >
> > Hi Daivd,
> >
> > I included this in my v5 of the IOMMU PMU series, but we noticed that
> > sashiko-bot AI reported two issues:
>
> I did say 'something like this' might work....
>

Got it! Thanks for clarifying. Just to share with you and everyone
here, I will go with the previous version. Thanks again!

> David
>
> >
> > 1: The lo_prev is immediately overwritten:
> > The trailing 'pmu->lo_prev[idx] = lo;' is outside the if block and
> > runs unconditionally, so the 'lo_prev = ~0u' is clobbered before the
> > function even returns. It never survives to the next call. lo_prev
> > ends up holding the guessed value (typically 0) instead, which is a
> > perfectly ordinary small number, and the intended "always re-verify
> > next time" behaviour never happens. The sentinel write was effectively
> > dead code.
> >
> > 2: The wrap check itself can miss a wrap
> > Here is a concrete sequence. Assume a previous call left hi_prev = 1
> > and lo_prev = 5, i.e. the counter was 0x1_00000005. Some time later
> > the counter has advanced close to 0x1_FFFFFFFF:
> >
> >   1. hi = readl(addr + 4)  -> counter is 0x1_FFFFFFF0, so hi = 1
> >   2. the counter crosses the boundary and becomes 0x2_00000008
> >   3. lo = readl(addr)      -> lo = 8
> >
> > The check then evaluates:
> >
> >   lo (8) <= lo_prev (5)   -> false
> >   hi (1) != hi_prev (1)   -> false
> >
> > Both are false, so the fast path is taken and 0x1_00000008 is
> > returned. The true value is 0x2_00000008, so the result is short by
> > 2^32 and the third read of the high half never even executes.
> >
> > Both situations share the same root cause: It puts the reliable check
> > (the third read) inside an if guarded by the unreliable cross-call
> > heuristic 'lo <= lo_prev || hi != hi_prev)'
> > Case 1 is an issue in the bookkeeping that heuristic depends on, and
> > case 2 shows that even with correct bookkeeping the heuristic is not
> > sound.
> >
> > To make everything simpler, I would use your first version for the
> > latest IOMMU PMU series:
> >
> >         hi = read_hi();
> >         lo = read_lo();
> >         if (hi != read_hi()) {
> >                 // Pick a value that happened while doing the reads.
> >                 hi++;
> >                 lo = 0;
> >         }
> >
> > > It shouldn't need any locking but the accesses do need to be ordered.
> > >
> > >         David
> > >
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv