Message-ID: <CAH2o1u5bNDpP1Lv1AicEe6uyi5X-ANiApxpvmfBgcxzg4ipH_Q@mail.gmail.com>
Date: Tue, 27 Aug 2024 10:34:56 -0700
From: Tomasz Jeznach <tjeznach@...osinc.com>
To: Jessica Clarke <jrtc27@...c27.com>
Cc: Xu Lu <luxu.kernel@...edance.com>, Anup Patel <apatel@...tanamicro.com>, 
	Albert Ou <aou@...s.berkeley.edu>, linux@...osinc.com, Will Deacon <will@...nel.org>, 
	joro@...tes.org, LKML <linux-kernel@...r.kernel.org>, 
	Yongji Xie <xieyongji@...edance.com>, iommu@...ts.linux.dev, 
	Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, wangqian.rd@...edance.com, 
	linux-riscv <linux-riscv@...ts.infradead.org>, robin.murphy@....com, 
	Hangjing Li <lihangjing@...edance.com>, baolu.lu@...ux.intel.com
Subject: Re: [External] Some feedbacks on RISC-V IOMMU driver

Hi Jessica and Xu,

On Mon, Aug 19, 2024 at 11:27 AM Jessica Clarke <jrtc27@...c27.com> wrote:
>
> On 19 Aug 2024, at 08:49, Xu Lu <luxu.kernel@...edance.com> wrote:
> >
> > Hi Jessica,
> >
> > On Mon, Aug 19, 2024 at 1:17 PM Jessica Clarke <jrtc27@...c27.com> wrote:
> >>
> >> On 19 Aug 2024, at 04:56, Xu Lu <luxu.kernel@...edance.com> wrote:
> >>>
> >>> Hi Tomasz,
> >>>
> >>> Thanks for your brilliant job on RISC-V IOMMU driver. It helps us a
> >>> lot for what we are doing. Below is our feedback on the existing
> >>> implementation[1].
> >>>
> >>> 1) Some IOMMU HW may only support 32-bit granularity access on its
> >>> control registers (even when the register is 8 byte length). Maybe it
> >>> is better to provide a 32-bit access method for 8 byte length
> >>> registers like what opensbi does on ACLINT MTIME register.
> >>
> >> That OpenSBI has to access MTIME piecewise is a workaround for a vendor
> >> not implementing what the spec clearly intended, even if it wasn’t
> >> explicitly stated (but is now, in response to that). Repeating that
> >> situation would be a pitiful mistake.
> >>
> >> The current IOMMU spec draft very clearly states:
> >>
> >>  "Registers that are 64-bit wide may be accessed using either a 32-bit
> >>   or a 64-bit access.”
> >>
> >
> > The spec's description about this is pretty confusing.
> > "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."
> > It seems that there is no requirement to implement 8-byte access.
> > It's OK then if we think this is not a problem.
>
> I agree that part is poorly worded. The part I quoted (from Software
> guidelines) is clear on the matter though. My interpretation of the
> part you quoted is that the spec has been written such that software
> can choose to break 8-byte accesses into two 4-byte ones and things
> will work, but if it doesn’t (which is a valid driver implementation),
> hardware can also do so should it not want to support an 8-byte
> interface (e.g. you have a 4-byte I/O bus).
>

I'd add to the quoted spec the remaining part of the sentence:
"The 8 byte IOMMU registers .... 8 byte access, to the high and low
halves of the register as long as the register semantics, with regards
to side-effects, are respected between the two software accesses, or
two hardware transactions, respectively"

If I/O bus limits access to 4-byte transfers, respecting described
ordering: "the high and low halves", should not create any
side-effects on the IOMMU hardware (mostly DDTP register access). If
there is a use case where hardware is not able to split 8-byte access
into 4-byte transactions, macros defined in io-64-nonatomic-hi-lo.h
would help here and other device drivers.

> Jess
>
> >> Jess
> >>
> >>> 2) In the IOMMU fault queue handling procedure, I wonder whether it is
> >>> better to clear the fqmf/fqof bit first, and then clear the ipsr.fip
> >>> bit. Otherwise the ipsr.fip can not be cleared and a redundant
> >>> interrupt will be signaled.
> >
> > By the way, it seems the irq handler must clear ipsr.fip first to
> > avoid missing out some faults no matter whether a redundant irq will
> > be generated.
> >
> > If ipsr.fiq pending via fqof/fqmf is implemented as edge triggering,
> > then ipsr.fiq can be cleared at first. In this case we can not clear
> > ipsr.fip again after clearing fqof/fqmf bit, as it indicates a new
> > fault to be handled.
> >
> > If ipsr.fiq pending via fqof/fqmf is implemented as level triggering,
> > then ipsr.fiq can not be cleared at first and a redundant irq will be
> > generated after this handler's return. But it is OK as no fault will
> > be missed. Otherwise it is hard to detect whether the ipsr.fiq is an
> > old one or a new one.
> >
> > Please correct me if I have any misunderstanding. Looking forward to
> > the subsequent code.
> >

Correct. The goal of clearing FIP first was not to miss fault,
accepting redundant interrupts to be generated in hopefully rare
cases. To be honest, handling fqof/fqmf faults will probably trigger
endpoint and/or IOMMU device quiesce anyway, and very likely more
fault reports, as the state of the device/system in such cases is
already suspected.

> > Best regards!
> >
> >>>
> >>> Best regards!
> >>> Xu Lu
> >>>
> >>> [1] https://lore.kernel.org/all/cover.1718388908.git.tjeznach@rivosinc.com/
> >>>
> >>> _______________________________________________
> >>> linux-riscv mailing list
> >>> linux-riscv@...ts.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>

Thank you for your feedback, and sorry for the late reply, still
catching up on emails.

Best,
- Tomasz

Powered by blists - more mailing lists