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

[Sai Prakash Ranjan]

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@xxxxxxxxxxxxxx>
> > > > ---
> > > > Â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