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

From: Julien Grall <julien@xen.org>
To: Rahul Singh <Rahul.Singh@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 4/8] xen/arm: Remove support for MSI on SMMUv3
Date: Wed, 2 Dec 2020 14:11:06 +0000	[thread overview]
Message-ID: <5689cfe7-ca16-6540-d394-00d3f60f4f5f@xen.org> (raw)
In-Reply-To: <D79D7DC5-649D-4517-A8CA-B13632595DA5@arm.com>

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