[External] Re: Some feedbacks on RISC-V IOMMU driver

[Xu Lu]

Hi Jessica,

On Mon, Aug 19, 2024 at 1:17 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 19 Aug 2024, at 04:56, Xu Lu <luxu.kernel at bytedance.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.

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

Best regards!

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