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

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

[Sai Prakash Ranjan]

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(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 8a908c50c306..634da02fef78 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -4142,11 +4142,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static void arm_smmu_device_shutdown(struct platform_device *pdev)
-{
-	arm_smmu_device_remove(pdev);
-}
-
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v3", },
 	{ },
@@ -4161,7 +4156,6 @@ static struct platform_driver arm_smmu_driver = {
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove	= arm_smmu_device_remove,
-	.shutdown = arm_smmu_device_shutdown,
 };
 module_platform_driver(arm_smmu_driver);
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..4d80516c789f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2276,11 +2276,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static void arm_smmu_device_shutdown(struct platform_device *pdev)
-{
-	arm_smmu_device_remove(pdev);
-}
-
 static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
 {
 	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
@@ -2335,7 +2330,6 @@ static struct platform_driver arm_smmu_driver = {
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove	= arm_smmu_device_remove,
-	.shutdown = arm_smmu_device_shutdown,
 };
 module_platform_driver(arm_smmu_driver);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Will Deacon]

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?

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.

Will

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

[Sai Prakash Ranjan]

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.

> 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?
As you see, with this SMMU shutdown callback, we need to add shutdown
callbacks in all the client drivers.

Thanks,
Sai

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

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

[Robin Murphy]

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.

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

[Will Deacon]

On Mon, Jun 08, 2020 at 02:43:03PM +0530, Sai Prakash Ranjan wrote:
> 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.

I'm not sure why you're trying to treat these cases differently. It's also
not "just because of SMMU", is it? Like I said, kexec() would be broken
regardless.

The bottom line is that after running ->shutdown() (or skipping it if it's
not implemented) for a driver, then the device must no longer perform DMA.

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

Given that the SMMU mediates DMA transactions for all upstream masters
based on in-memory data (e.g. page tables), then I think it's a /very/
good idea to tear that down as part of the shutdown callback before
the memory is effectively free()d.

One thing I would be in favour of is changing the ->shutdown() code to
honour disable_bypass=1 so that we put the SMMU in an aborting state
instead of passthrough. Would that help at all? It would at least
avoid the memory corruption on missing shutdown callback.

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

I don't see the problem with that. Why is it a problem?

Will

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

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

[Sai Prakash Ranjan]

Hi Will,

On 2020-06-08 17:08, Will Deacon wrote:
> On Mon, Jun 08, 2020 at 02:43:03PM +0530, Sai Prakash Ranjan wrote:
>> 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.
> 
> I'm not sure why you're trying to treat these cases differently. It's 
> also
> not "just because of SMMU", is it? Like I said, kexec() would be broken
> regardless.
> 
> The bottom line is that after running ->shutdown() (or skipping it if 
> it's
> not implemented) for a driver, then the device must no longer perform 
> DMA.
> 

Yes it's not just because of SMMU. Now I understand that we indeed need
to shutdown SMMU like any other driver and we need to fix the client
drivers as well.

>> > 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?
> 
> Given that the SMMU mediates DMA transactions for all upstream masters
> based on in-memory data (e.g. page tables), then I think it's a /very/
> good idea to tear that down as part of the shutdown callback before
> the memory is effectively free()d.
> 
> One thing I would be in favour of is changing the ->shutdown() code to
> honour disable_bypass=1 so that we put the SMMU in an aborting state
> instead of passthrough. Would that help at all? It would at least
> avoid the memory corruption on missing shutdown callback.
> 

This would be good, however this would then mask the fact that it is
client drivers who are buggy and if we stop seeing issues, then no one
will bother fixing the client drivers. So we better go ahead and fix
the client drivers first and I will drop this patch.

>> As you see, with this SMMU shutdown callback, we need to add shutdown
>> callbacks in all the client drivers.
> 
> I don't see the problem with that. Why is it a problem?
> 

Not a problem, I wanted to confirm if kexec issue was indeed only due to
client driver bugs or SMMU has also some contribution. We have already
started adding client driver shutdown callbacks [1][2], but I also
wanted to get this clarified, so sent this patch as RFC.

[1] - 
https://lore.kernel.org/lkml/1591009402-681-1-git-send-email-mkrishn@codeaurora.org/
[2] - 
https://lore.kernel.org/lkml/28123d1e19f235f97555ee36a5ed8b52d20cbdea.1590947174.git.saiprakash.ranjan@codeaurora.org/

Thanks,
Sai

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