Re: [PATCH v2 4/8] xen/arm: Remove support for MSI on SMMUv3

[Julien Grall <julien@xxxxxxx>]

Hi Rahul,

On 02/12/2020 13:12, Rahul Singh wrote:
> Hello Stefano,
>
> > On 2 Dec 2020, at 12:40 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> >
> > On Tue, 1 Dec 2020, Stefano Stabellini wrote:
> > > On Thu, 26 Nov 2020, Rahul Singh wrote:
> > > > XEN does not support MSI on ARM platforms, therefore remove the MSI
> > > > support from SMMUv3 driver.
> > > >
> > > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > I wonder if it makes sense to #ifdef CONFIG_MSI this code instead of
> > > removing it completely.
> > One more thought: could this patch be achieved by reverting
> > 166bdbd23161160f2abcea70621adba179050bee ? If this patch could be done
> > by a couple of revert, it would be great to say it in the commit
> > message.
>   Ok will add in next version.
>
> > > In the past, we tried to keep the entire file as textually similar to
> > > the original Linux driver as possible to make it easier to backport
> > > features and fixes. So, in this case we would probably not even used an
> > > #ifdef but maybe something like:
> > >
> > >   if (/* msi_enabled */ 0)
> > >       return;
> > >
> > > at the beginning of arm_smmu_setup_msis.
> > >
> > >
> > > However, that strategy didn't actually work very well because backports
> > > have proven difficult to do anyway. So at that point we might as well at
> > > least have clean code in Xen and do the changes properly.
It was difficult because Linux decided to rework how IOMMU drivers 
works. I agree the risk is still there and therefore clean code would be 
better with some caveats (see below).
> Main reason to remove the feature/code that is not usable in XEN is to have a 
> clean code.
I agree that short term this feature will not be usable. However, I 
think we need to think about {medium, long}-term plan to avoid extra 
effort in the future because the driver evolve in a way making the 
revert of revert impossible.
Therefore I would prefer to keep both the MSI and PCI ATS present as 
they are going to be useful/necessary on some platforms. It doesn't 
matter that they don't work because the driver will be in tech preview.
Cheers,

--
Julien Grall