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

[Julien Grall <julien@xen.org>]

Hi Rahul,

On 02/12/2020 13:12, Rahul Singh wrote:
> Hello Stefano,
> 
>> On 2 Dec 2020, at 12:40 am, Stefano Stabellini <sstabellini@kernel.org> 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@arm.com>
>>>
>>> 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