[External] Some feedbacks on RISC-V IOMMU driver

[Jessica Clarke]

On 19 Aug 2024, at 08:49, Xu Lu <luxu.kernel at bytedance.com> wrote:
> 
> 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.

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

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