lkml.org 
[lkml]   [2020]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] iommu/arm-smmu: Remove shutdown callback
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

\
 
 \ /
  Last update: 2020-06-08 15:51    [W:0.109 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site