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

[Julien Grall <julien@xxxxxxx>]

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

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

Cheers,

--
Julien Grall