Messages in this thread | | | Date | Mon, 08 Jun 2020 19:20:17 +0530 | From | Sai Prakash Ranjan <> | Subject | Re: [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
| |