Re: [RFC PATCH] iommu/arm-smmu: Remove shutdown callback

[Robin Murphy <robin.murphy@arm.com>]

On 2020-06-08 10:13, Sai Prakash Ranjan wrote:
> Hi Will,
> 
> On 2020-06-08 13:48, Will Deacon wrote:
>> On Sun, Jun 07, 2020 at 04:39:18PM +0530, Sai Prakash Ranjan wrote:
>>> Remove SMMU shutdown callback since it seems to cause more
>>> problems than benefits. With this callback, we need to make
>>> sure that all clients/consumers of SMMU do not perform any
>>> DMA activity once the SMMU is shutdown and translation is
>>> disabled. In other words we need to add shutdown callbacks
>>> for all those clients to make sure they do not perform any
>>> DMA or else we see all kinds of weird crashes during reboot
>>> or shutdown. This is clearly not scalable as the number of
>>> clients of SMMU would vary across SoCs and we would need to
>>> add shutdown callbacks to almost all drivers eventually.
>>> This callback was added for kexec usecase where it was known
>>> to cause memory corruptions when SMMU was not shutdown but
>>> that does not directly relate to SMMU because the memory
>>> corruption could be because of the client of SMMU which is
>>> not shutdown properly before booting into new kernel. So in
>>> that case, we need to identify the client of SMMU causing
>>> the memory corruption and add appropriate shutdown callback
>>> to the client rather than to the SMMU.
>>>
>>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>> ---
>>>  drivers/iommu/arm-smmu-v3.c | 6 ------
>>>  drivers/iommu/arm-smmu.c    | 6 ------
>>>  2 files changed, 12 deletions(-)
>>
>> This feels like a giant bodge to me and I think that any driver which
>> continues to perform DMA after its ->shutdown() function has been invoked
>> is buggy. Wouldn't that cause problems with kexec(), for example?
>>
> 
> Yes it is definitely a bug in the client driver if DMA is performed
> after shutdown callback of that respective driver is invoked and it must
> be fixed in that driver. But here the problem I was describing is not that,
> most of the drivers do not have a shutdown callback to begin with and 
> adding
> it just because of shutdown dependency on SMMU doesn't seem so well because
> we can have many more such clients in the future and then we have to 
> just go
> on adding the shutdown callbacks everywhere.

Yes, indeed we do. Like it or not, kexec is a thing, and about 98% of 
drivers will have been written without considering it.

If fixing one problem exposes lots of other problems, can you honestly 
say that the best solution is to go back and re-break the original thing?

>> There's a clear shutdown dependency ordering, where the clients of the
>> SMMU need to shutdown before the SMMU itself, but that's not really
>> the SMMU driver's problem to solve.
>>
> 
> The problem with kexec may not be directly related to SMMU as you said
> above if DMA is performed after the client shutdown callback, then its a
> bug in the client driver, so that needs to be fixed in the client driver,
> not the SMMU. So is there any point in having the SMMU shutdown callback?

The point is that kexec needs to return the system to a state as close 
to reset as possible. The SMMU is at least as relevant as any other 
device in that regard, if not far more so - consider if the first kernel 
*did* leave it enabled with whatever left-over translations in place, 
and kexec'ed into another OS that wasn't SMMU-aware...

> As you see, with this SMMU shutdown callback, we need to add shutdown
> callbacks in all the client drivers.

Yes. And if you really want to argue against that, then at least be 
consistent and propose removing shutdown from *all* the IOMMU drivers. 
And then probably propose removing kexec support from all their 
respective architectures... ;)

Robin.