Re: [XEN PATCH v1] xen/arm : Add support for SMMUv3 driver

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 20 Oct 2020, at 6:03 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> Thank you for the contribution. Lets make sure this attempt to SMMUv3 support 
> in Xen will be more successful than the other one :).

Yes sure.
> 
> I haven't reviewed the code yet, but I wanted to provide feedback on the 
> commit message.
> 
> On 20/10/2020 16:25, Rahul Singh wrote:
>> Add support for ARM architected SMMUv3 implementations. It is based on
>> the Linux SMMUv3 driver.
>> Major differences between the Linux driver are as follows:
>> 1. Only Stage-2 translation is supported as compared to the Linux driver
>>    that supports both Stage-1 and Stage-2 translations.
>> 2. Use P2M  page table instead of creating one as SMMUv3 has the
>>    capability to share the page tables with the CPU.
>> 3. Tasklets is used in place of threaded IRQ's in Linux for event queue
>>    and priority queue IRQ handling.
> 
> Tasklets are not a replacement for threaded IRQ. In particular, they will 
> have priority over anything else (IOW nothing will run on the pCPU until they 
> are done).
> 
> Do you know why Linux is using thread. Is it because of long running 
> operations?

Yes you are right because of long running operations Linux is using the 
threaded IRQs. 

SMMUv3 reports fault/events bases on memory-based circular buffer queues not 
based on the register. As per my understanding, it is time-consuming to process 
the memory based queues in interrupt context because of that Linux is using 
threaded IRQ to process the faults/events from SMMU.

I didn’t find any other solution in XEN in place of tasklet to defer the work, 
that’s why I used tasklet in XEN in replacement of threaded IRQs. If we do all 
work in interrupt context we will make XEN less responsive.

If you know another solution in XEN that will be used to defer the work in the 
interrupt please let me know I will try to use that.

>> 4. Latest version of the Linux SMMUv3 code implements the commands queue
>>    access functions based on atomic operations implemented in Linux.
> 
> Can you provide more details?

I tried to port the latest version of the SMMUv3 code than I observed that in 
order to port that code I have to also port atomic operation implemented in 
Linux to XEN. As latest Linux code uses atomic operation to process the command 
queues (atomic_cond_read_relaxed(),atomic_long_cond_read_relaxed() , 
atomic_fetch_andnot_relaxed()) .

> 
>>    Atomic functions used by the commands queue access functions is not
>>    implemented in XEN therefore we decided to port the earlier version
>>    of the code. Once the proper atomic operations will be available in XEN
>>    the driver can be updated.
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>>  xen/drivers/passthrough/Kconfig       |   10 +
>>  xen/drivers/passthrough/arm/Makefile  |    1 +
>>  xen/drivers/passthrough/arm/smmu-v3.c | 2847 +++++++++++++++++++++++++
>>  3 files changed, 2858 insertions(+)
> 
> This is quite significant patch to review. Is there any way to get it split 
> (maybe a verbatim Linux copy + Xen modification)?

Yes, I understand this is a quite significant patch to review let me think to 
get it split. If it is ok for you to review this patch and provide your 
comments then it will great for us.

> 
> Cheers,
> 
> -- 
> Julien Grall