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

[Julien Grall <julien@xxxxxxx>]

On 30/10/2020 11:33, Rahul Singh wrote:
> Hello Julien,
Hi,

> > On 30 Oct 2020, at 10:05 am, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 30/10/2020 09:45, Rahul Singh wrote:
> > > Hello Julien,
> > > > On 30 Oct 2020, at 9:21 am, Julien Grall <julien@xxxxxxx> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 30/10/2020 08:46, Rahul Singh wrote:
> > > > > Ok Yes when I ported the driver I port the command queue operation from the 
> > > > > previous commit where atomic operations is not used and rest all the code is 
> > > > > from the latest code. I will again make sure that any bug that is fixed in 
> > > > > Linux should be fixed in XEN also.
> > > > I would like to seek some clarifications on the code because there seem to be 
> > > > conflicting information provided in this thread.
> > > >
> > > > The patch (the baseline commit is provided) and the discussion with Bertrand 
> > > > suggests that you took a snapshot of the code last year and adapted for Xen.
> > > >
> > > > However, here you suggest that you took an hybrid approach where part of the 
> > > > code is based from last year and other part is based from the latest code (I 
> > > > assume v5.9).
> > > >
> > > > So can you please clarify?
> > > >
> > > > Cheers,
> > > Approach I took is to first merge the code  from the commit ( Jul 2, 2019 
> > > 7c288a5b27934281d9ea8b5807bc727268b7001a ) the snapshot before atomic operation 
> > > is used in SMMUv3 code for command queue operations.
> > > After that I fixed  the other code( not related to command queue operations.)  
> > > from the latest code so that no bug is introduced in XEN because of using the 
> > > last year commit.
> > Ok. That was definitely not clear from the commit message. Please make this 
> > clearer in the commit message.
> Ok. I will make this clearer in the commit message.
>
> > Anway, it means we need to do a full review of the code (rather than a light 
> > one) because of the hybrid model.
> >
> > I am still a bit puzzle to why it would require almost of a restart of the 
> > implementation in order to sync the latest code. Does it imply that you are 
> > mostly using the old code?
> SMMuv3 code is divided into below parts :
>
> 1. Low-level/High level queue manipulation functions.
> 2. Context descriptor manipulation functions.
> 3. Stream table manipulation functions.
> 4. Interrupt handling.
> 5. Linux IOMMU API functions.
> 6. Driver initialisation functions( probe/reset ).
>
> Low-level/High-level queue manipulation functions are from the old code, rest 
> is the new code whenever it was possible.
Thanks for the details! I think it would be useful to mention that in 
the commit message.
> I started with porting the latest code but there are many dependencies for the 
> queue manipulation function so we decided to use the old queue manipulation 
> function.
In general, I would recommend to involve the community as soon as 
possible in the development process. This is quite important for big 
feature because it allows all the party to agree on the approach without 
investing significant effort.
> As the queue manipulation function is a big part of the code it will require a 
> lot of effort and testing to sync with the latest code once the atomic 
> operation is in place to use
Sure, everything has a cost (including maintaining the code). This has 
to be a trade-off.
My main concern was the maintenance of the code long term. However, as 
you and Bertrand stepped up for maintaining the code, then this should 
be less of a concern...
Cheers,

--
Julien Grall