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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 22 Oct 2020, at 9:32 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 21/10/2020 12:25, Rahul Singh wrote:
>> Hello Julien,
> 
> Hi Rahul,
> 
>>> 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.
> 
> So we need to make sure that Xen continue to receives interrupts, but we also 
> need to make sure that a vCPU bound to the pCPU is also responsive.

Yes I agree.
> 
>> 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.
> 
> One of my work colleague encountered a similar problem recently. He had a 
> long running tasklet and wanted to be broken down in smaller chunk.
> 
> We decided to use a timer to reschedule the taslket in the future. This 
> allows the scheduler to run other loads (e.g. vCPU) for some time.
> 
> This is pretty hackish but I couldn't find a better solution as tasklet have 
> high priority.
> 
> Maybe the other will have a better idea.

Let me try to use the timer and will share my findings.
> 
>>>> 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()) .
> 
> Thank you for the explanation. I think it would be best to import the atomic 
> helpers and use the latest code.
> 
> This will ensure that we don't re-introduce bugs and also buy us some time 
> before the Linux and Xen driver diverge again too much.
> 
> Stefano, what do you think?
> 
>>> 
>>>>    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.
> I will try to have a look next week.

Thanks in advance ☺️
> 
> Cheers,
> 
> -- 
> Julien Grall

Regards,
Rahul