lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPYmKFv_qRmsR+4s3evxCJjzVavtNb6UFRX6+JmepUW23X=ZLg@mail.gmail.com>
Date: Wed, 2 Apr 2025 19:58:38 +0800
From: Xu Lu <luxu.kernel@...edance.com>
To: Robin Murphy <robin.murphy@....com>
Cc: tjeznach@...osinc.com, joro@...tes.org, will@...nel.org, 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 Robin and Jason,

Thanks for your comments and sorry for the late reply.

The iohpmcycles register is handled specially because unlike the other
IOMMU registers, its effective bits lies in the upper half. I want to
ensure that all effective bits are the last ones to update. For
example, when updating DDTP register, we update the upper half first
which only contains its table PPN, and then update its lower half
which contains the MODE bits which really enable translation. The main
reason is that when software splits an 8-byte write to two 4-byte
writes, the hardware is not aware whether it is an 8-byte write or a
4-byte write when it receives the first write. Thus it will always
make the value effective every time it receives a 4-byte write.

On Wed, Apr 2, 2025 at 7:29 PM Robin Murphy <robin.murphy@....com> wrote:
>
> On 2025-03-25 2:42 pm, Xu Lu 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>
> > ---
> >   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)))
>
> Echoing Jason's comment, what is this even trying to achieve? Nothing in
> the spec suggests that the cycle counter register is functionally
> different from the other PMU counter registers (other than its
> self-contained overflow bit).
>
> It is not, in general, safe to do a split write to a running counter
> either way - low-high vs. high-low just moves the problem around,
> changing *which* combinations of values are problematic and capable of
> overflowing into each other between the writes. If the PMU driver can't
> write counters atomically, it will need to ensure that it only ever
> write them while stopped (at which point the order surely shouldn't
> matter). Conversely, though, reading from running counters is a bit more
> reasonable, but it needs more than just hi_lo_readq to guarantee it's
> not got a torn result.

You are right. It is more appropriate to ensure update iohpmcycles
with cycle counting disabled. I will remove this redundant branch in
the next version.

>
> Thanks,
> Robin.
>
> > +
> > +#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
>

Best regards,
Xu Lu

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ