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

[Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>]

Hi Robin,

On 2020-06-08 16:56, Robin Murphy wrote:
> 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?
> 

No, definitely not. I was under the wrong impression that *kexec thing*
was more due to the client driver bugs rather than the SMMU.

>>> 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...
> 

Yes understood. I wrongly assumed that the kexec problem was more
of a client driver bugs rather than SMMU. But I see that we indeed
need to shutdown SMMU as any other driver.

>> 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... ;)
> 

Not my intention to break or remove kexec, hence the RFC tag :)
As for the consistent part, out of 18 iommu drivers only 5
have shutdown callbacks, so nothing much to remove there and
kexec is broken there probably. However I got your point and
will drop this patch.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation