[RFC,06/11] iommu: arm-smmu: Remove Calxeda secure mode quirk

Commit Message

Rob Herring (Arm) Feb. 18, 2020, 5:13 p.m. UTC

Cc: Will Deacon <[e-mail]>
Cc: Robin Murphy <[e-mail]>
Cc: Joerg Roedel <[e-mail]>
Cc: [e-mail]
Signed-off-by: Rob Herring <[e-mail]>
---
Do not apply yet.

 drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
 1 file changed, 43 deletions(-)

--
2.20.1

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#1404): https://linux.kernel.org/g/patchwork-soc/message/1404
Mute This Topic: https://linux.kernel.org/mt/71375432/1554929
Group Owner: [e-mail]
Unsubscribe: https://linux.kernel.org/g/patchwork-soc/unsub  [[e-mail]]
-=-=-=-=-=-=-=-=-=-=-=-

Comments

Will Deacon Feb. 18, 2020, 5:20 p.m. UTC | #1

On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:
> Cc: Will Deacon <[e-mail]>
> Cc: Robin Murphy <[e-mail]>
> Cc: Joerg Roedel <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Rob Herring <[e-mail]>
> ---
> Do not apply yet.

Pleeeeease? ;)

>  drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
>  1 file changed, 43 deletions(-)

Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
anything from 'struct arm_smmu_impl' because most implementations fall
just short of perfect.

Anyway, let me know when I can push the button and I'll queue this in
the arm-smmu tree.

Cheers,

Will

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#1410): https://linux.kernel.org/g/patchwork-soc/message/1410
Mute This Topic: https://linux.kernel.org/mt/71375432/1554929
Group Owner: [e-mail]
Unsubscribe: https://linux.kernel.org/g/patchwork-soc/unsub  [[e-mail]]
-=-=-=-=-=-=-=-=-=-=-=-

Robin Murphy Feb. 18, 2020, 5:32 p.m. UTC | #2

On 18/02/2020 5:20 pm, Will Deacon wrote:
> On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:
>> Cc: Will Deacon <[e-mail]>
>> Cc: Robin Murphy <[e-mail]>
>> Cc: Joerg Roedel <[e-mail]>
>> Cc: [e-mail]
>> Signed-off-by: Rob Herring <[e-mail]>
>> ---
>> Do not apply yet.
> 
> Pleeeeease? ;)
> 
>>   drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------

Presumably we also want to remove the definition of the option from 
binding too.

>>   1 file changed, 43 deletions(-)
> 
> Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
> anything from 'struct arm_smmu_impl' because most implementations fall
> just short of perfect.

Right, this served as the prototype for register access hooks, but we 
have at least one other known user for those.

> Anyway, let me know when I can push the button and I'll queue this in
> the arm-smmu tree.

FWIW the quirk has proven useful in other circumstances too, but I 
imagine if we ever have to prototype an integration on VExpress-CA9 
again, reverting this patch will hardly be the most unpleasant part :)

Robin.

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#1413): https://linux.kernel.org/g/patchwork-soc/message/1413
Mute This Topic: https://linux.kernel.org/mt/71375432/1554929
Group Owner: [e-mail]
Unsubscribe: https://linux.kernel.org/g/patchwork-soc/unsub  [[e-mail]]
-=-=-=-=-=-=-=-=-=-=-=-

Rob Herring (Arm) Feb. 25, 2020, 10:01 p.m. UTC | #3

On Tue, Feb 18, 2020 at 11:20 AM Will Deacon <[e-mail]> wrote:
>
> On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:
> > Cc: Will Deacon <[e-mail]>
> > Cc: Robin Murphy <[e-mail]>
> > Cc: Joerg Roedel <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Rob Herring <[e-mail]>
> > ---
> > Do not apply yet.
>
> Pleeeeease? ;)
>
> >  drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
> >  1 file changed, 43 deletions(-)
>
> Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
> anything from 'struct arm_smmu_impl' because most implementations fall
> just short of perfect.
>
> Anyway, let me know when I can push the button and I'll queue this in
> the arm-smmu tree.

Seems we're leaving the platform support for now, but I think we never
actually enabled SMMU support. It's not in the dts either in mainline
nor the version I have which should be close to what shipped in
firmware. So as long as Andre agrees, this one is good to apply.

Rob

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#1440): https://linux.kernel.org/g/patchwork-soc/message/1440
Mute This Topic: https://linux.kernel.org/mt/71375432/1554929
Group Owner: [e-mail]
Unsubscribe: https://linux.kernel.org/g/patchwork-soc/unsub  [[e-mail]]
-=-=-=-=-=-=-=-=-=-=-=-

Will Deacon Feb. 28, 2020, 10:04 a.m. UTC | #4

On Tue, Feb 25, 2020 at 04:01:54PM -0600, Rob Herring wrote:
> On Tue, Feb 18, 2020 at 11:20 AM Will Deacon <[e-mail]> wrote:
> >
> > On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:
> > > Cc: Will Deacon <[e-mail]>
> > > Cc: Robin Murphy <[e-mail]>
> > > Cc: Joerg Roedel <[e-mail]>
> > > Cc: [e-mail]
> > > Signed-off-by: Rob Herring <[e-mail]>
> > > ---
> > > Do not apply yet.
> >
> > Pleeeeease? ;)
> >
> > >  drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
> > >  1 file changed, 43 deletions(-)
> >
> > Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
> > anything from 'struct arm_smmu_impl' because most implementations fall
> > just short of perfect.
> >
> > Anyway, let me know when I can push the button and I'll queue this in
> > the arm-smmu tree.
> 
> Seems we're leaving the platform support for now, but I think we never
> actually enabled SMMU support. It's not in the dts either in mainline
> nor the version I have which should be close to what shipped in
> firmware. So as long as Andre agrees, this one is good to apply.

Andre? Can I queue this one for 5.7, please?

Will

Andre Przywara Feb. 28, 2020, 10:25 a.m. UTC | #5

On Fri, 28 Feb 2020 10:04:47 +0000
Will Deacon <[e-mail]> wrote:

Hi,

> On Tue, Feb 25, 2020 at 04:01:54PM -0600, Rob Herring wrote:
> > On Tue, Feb 18, 2020 at 11:20 AM Will Deacon <[e-mail]> wrote:  
> > >
> > > On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:  
> > > > Cc: Will Deacon <[e-mail]>
> > > > Cc: Robin Murphy <[e-mail]>
> > > > Cc: Joerg Roedel <[e-mail]>
> > > > Cc: [e-mail]
> > > > Signed-off-by: Rob Herring <[e-mail]>
> > > > ---
> > > > Do not apply yet.  
> > >
> > > Pleeeeease? ;)
> > >  
> > > >  drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
> > > >  1 file changed, 43 deletions(-)  
> > >
> > > Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
> > > anything from 'struct arm_smmu_impl' because most implementations fall
> > > just short of perfect.
> > >
> > > Anyway, let me know when I can push the button and I'll queue this in
> > > the arm-smmu tree.  
> > 
> > Seems we're leaving the platform support for now, but I think we never
> > actually enabled SMMU support. It's not in the dts either in mainline
> > nor the version I have which should be close to what shipped in
> > firmware. So as long as Andre agrees, this one is good to apply.  
> 
> Andre? Can I queue this one for 5.7, please?

I was wondering how much of a pain it is to keep it in? AFAICS there are other users of the "impl" indirection. If those goes away, I would be happy to let Calxeda go.
But Eric had the magic DT nodes to get the SMMU working, and I used that before, with updating the DT either on flash or dynamically via U-Boot.

So I don't know exactly *how* desperate you are with removing this, or if there are other reasons than "negative diffstat", but if possible I would like to keep it in.

Cheers,
Andre.

Will Deacon Feb. 28, 2020, 10:50 a.m. UTC | #6

On Fri, Feb 28, 2020 at 10:25:56AM +0000, Andre Przywara wrote:
> On Fri, 28 Feb 2020 10:04:47 +0000
> Will Deacon <[e-mail]> wrote:
> 
> Hi,
> 
> > On Tue, Feb 25, 2020 at 04:01:54PM -0600, Rob Herring wrote:
> > > On Tue, Feb 18, 2020 at 11:20 AM Will Deacon <[e-mail]> wrote:  
> > > >
> > > > On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:  
> > > > > Cc: Will Deacon <[e-mail]>
> > > > > Cc: Robin Murphy <[e-mail]>
> > > > > Cc: Joerg Roedel <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > Signed-off-by: Rob Herring <[e-mail]>
> > > > > ---
> > > > > Do not apply yet.  
> > > >
> > > > Pleeeeease? ;)
> > > >  
> > > > >  drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
> > > > >  1 file changed, 43 deletions(-)  
> > > >
> > > > Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
> > > > anything from 'struct arm_smmu_impl' because most implementations fall
> > > > just short of perfect.
> > > >
> > > > Anyway, let me know when I can push the button and I'll queue this in
> > > > the arm-smmu tree.  
> > > 
> > > Seems we're leaving the platform support for now, but I think we never
> > > actually enabled SMMU support. It's not in the dts either in mainline
> > > nor the version I have which should be close to what shipped in
> > > firmware. So as long as Andre agrees, this one is good to apply.  
> > 
> > Andre? Can I queue this one for 5.7, please?
> 
> I was wondering how much of a pain it is to keep it in? AFAICS there are
> other users of the "impl" indirection. If those goes away, I would be
> happy to let Calxeda go.

The impl stuff is new, so we'll keep it around. The concern is more about
testing (see below).

> But Eric had the magic DT nodes to get the SMMU working, and I used that
> before, with updating the DT either on flash or dynamically via U-Boot.

What did you actually use the SMMU for, though? The
'arm_iommu_create_mapping()' interface isn't widely used and, given that
highbank doesn't support KVM, the use-cases for VFIO are pretty limited
too.

> So I don't know exactly *how* desperate you are with removing this, or if
> there are other reasons than "negative diffstat", but if possible I would
> like to keep it in.

It's more that we *do* make quite a lot of changes to the arm-smmu driver
and it's never tested with this quirk. If you're stepping up to run smmu
tests on my queue for each release on highbank, then great, but otherwise
I'd rather not carry the code for fun. The change in diffstat is minimal
(we're going to need to hooks for nvidia, who broke things in a different
way).

Also, since the hooks aren't going away, if you /do/ end up using the SMMU
in future, then we could re-add the driver quirk without any fuss.

Will

Andre Przywara Feb. 28, 2020, 1:42 p.m. UTC | #7

On Fri, 28 Feb 2020 10:50:25 +0000
Will Deacon <[e-mail]> wrote:

> On Fri, Feb 28, 2020 at 10:25:56AM +0000, Andre Przywara wrote:
> > On Fri, 28 Feb 2020 10:04:47 +0000
> > Will Deacon <[e-mail]> wrote:
> > 
> > Hi,
> >   
> > > On Tue, Feb 25, 2020 at 04:01:54PM -0600, Rob Herring wrote:  
> > > > On Tue, Feb 18, 2020 at 11:20 AM Will Deacon <[e-mail]> wrote:    
> > > > >
> > > > > On Tue, Feb 18, 2020 at 11:13:16AM -0600, Rob Herring wrote:    
> > > > > > Cc: Will Deacon <[e-mail]>
> > > > > > Cc: Robin Murphy <[e-mail]>
> > > > > > Cc: Joerg Roedel <[e-mail]>
> > > > > > Cc: [e-mail]
> > > > > > Signed-off-by: Rob Herring <[e-mail]>
> > > > > > ---
> > > > > > Do not apply yet.    
> > > > >
> > > > > Pleeeeease? ;)
> > > > >    
> > > > > >  drivers/iommu/arm-smmu-impl.c | 43 -----------------------------------
> > > > > >  1 file changed, 43 deletions(-)    
> > > > >
> > > > > Yes, I'm happy to get rid of this. Sadly, I don't think we can remove
> > > > > anything from 'struct arm_smmu_impl' because most implementations fall
> > > > > just short of perfect.
> > > > >
> > > > > Anyway, let me know when I can push the button and I'll queue this in
> > > > > the arm-smmu tree.    
> > > > 
> > > > Seems we're leaving the platform support for now, but I think we never
> > > > actually enabled SMMU support. It's not in the dts either in mainline
> > > > nor the version I have which should be close to what shipped in
> > > > firmware. So as long as Andre agrees, this one is good to apply.    
> > > 
> > > Andre? Can I queue this one for 5.7, please?  
> > 
> > I was wondering how much of a pain it is to keep it in? AFAICS there are
> > other users of the "impl" indirection. If those goes away, I would be
> > happy to let Calxeda go.  
> 
> The impl stuff is new, so we'll keep it around. The concern is more about
> testing (see below).
> 
> > But Eric had the magic DT nodes to get the SMMU working, and I used that
> > before, with updating the DT either on flash or dynamically via U-Boot.  
> 
> What did you actually use the SMMU for, though? The
> 'arm_iommu_create_mapping()' interface isn't widely used and, given that
> highbank doesn't support KVM, the use-cases for VFIO are pretty limited
> too.

AFAIK Highbank doesn't have the SMMU, probably mostly for that reason.
I have a DT snippet for Midway, and that puts the MMIO base at ~36GB, which is not possible on Highbank.
So I think that the quirk is really meant and needed for Midway.

> > So I don't know exactly *how* desperate you are with removing this, or if
> > there are other reasons than "negative diffstat", but if possible I would
> > like to keep it in.  
> 
> It's more that we *do* make quite a lot of changes to the arm-smmu driver
> and it's never tested with this quirk. If you're stepping up to run smmu
> tests on my queue for each release on highbank, then great, but otherwise
> I'd rather not carry the code for fun. The change in diffstat is minimal
> (we're going to need to hooks for nvidia, who broke things in a different
> way).

I am about to set up some more sophisticated testing, and will include some SMMU bits in it.

Cheers,
Andre.

> Also, since the hooks aren't going away, if you /do/ end up using the SMMU
> in future, then we could re-add the driver quirk without any fuss.
> 
> Will

Will Deacon Feb. 28, 2020, 1:56 p.m. UTC | #8

On Fri, Feb 28, 2020 at 01:42:54PM +0000, Andre Przywara wrote:
> On Fri, 28 Feb 2020 10:50:25 +0000
> Will Deacon <[e-mail]> wrote:
> > On Fri, Feb 28, 2020 at 10:25:56AM +0000, Andre Przywara wrote:
> > > > On Tue, Feb 25, 2020 at 04:01:54PM -0600, Rob Herring wrote:  
> > > > > Seems we're leaving the platform support for now, but I think we never
> > > > > actually enabled SMMU support. It's not in the dts either in mainline
> > > > > nor the version I have which should be close to what shipped in
> > > > > firmware. So as long as Andre agrees, this one is good to apply.    
> > > > 
> > > > Andre? Can I queue this one for 5.7, please?  
> > > 
> > > I was wondering how much of a pain it is to keep it in? AFAICS there are
> > > other users of the "impl" indirection. If those goes away, I would be
> > > happy to let Calxeda go.  
> > 
> > The impl stuff is new, so we'll keep it around. The concern is more about
> > testing (see below).
> > 
> > > But Eric had the magic DT nodes to get the SMMU working, and I used that
> > > before, with updating the DT either on flash or dynamically via U-Boot.  
> > 
> > What did you actually use the SMMU for, though? The
> > 'arm_iommu_create_mapping()' interface isn't widely used and, given that
> > highbank doesn't support KVM, the use-cases for VFIO are pretty limited
> > too.
> 
> AFAIK Highbank doesn't have the SMMU, probably mostly for that reason.
> I have a DT snippet for Midway, and that puts the MMIO base at ~36GB, which is not possible on Highbank.
> So I think that the quirk is really meant and needed for Midway.

Sorry, but I don't follow your reasoning here. The MMIO base has nothing
to do with the quirk, although doing some digging it looks like your
conclusion about this applying to Midway (ecx-2000?) is correct:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226095.html

> > > So I don't know exactly *how* desperate you are with removing this, or if
> > > there are other reasons than "negative diffstat", but if possible I would
> > > like to keep it in.  
> > 
> > It's more that we *do* make quite a lot of changes to the arm-smmu driver
> > and it's never tested with this quirk. If you're stepping up to run smmu
> > tests on my queue for each release on highbank, then great, but otherwise
> > I'd rather not carry the code for fun. The change in diffstat is minimal
> > (we're going to need to hooks for nvidia, who broke things in a different
> > way).
> 
> I am about to set up some more sophisticated testing, and will include
> some SMMU bits in it.

Yes, please.

Will

Andre Przywara Feb. 28, 2020, 2:11 p.m. UTC | #9

On Fri, 28 Feb 2020 13:56:46 +0000
Will Deacon <[e-mail]> wrote:

> On Fri, Feb 28, 2020 at 01:42:54PM +0000, Andre Przywara wrote:
> > On Fri, 28 Feb 2020 10:50:25 +0000
> > Will Deacon <[e-mail]> wrote:  
> > > On Fri, Feb 28, 2020 at 10:25:56AM +0000, Andre Przywara wrote:  
> > > > > On Tue, Feb 25, 2020 at 04:01:54PM -0600, Rob Herring wrote:    
> > > > > > Seems we're leaving the platform support for now, but I think we never
> > > > > > actually enabled SMMU support. It's not in the dts either in mainline
> > > > > > nor the version I have which should be close to what shipped in
> > > > > > firmware. So as long as Andre agrees, this one is good to apply.      
> > > > > 
> > > > > Andre? Can I queue this one for 5.7, please?    
> > > > 
> > > > I was wondering how much of a pain it is to keep it in? AFAICS there are
> > > > other users of the "impl" indirection. If those goes away, I would be
> > > > happy to let Calxeda go.    
> > > 
> > > The impl stuff is new, so we'll keep it around. The concern is more about
> > > testing (see below).
> > >   
> > > > But Eric had the magic DT nodes to get the SMMU working, and I used that
> > > > before, with updating the DT either on flash or dynamically via U-Boot.    
> > > 
> > > What did you actually use the SMMU for, though? The
> > > 'arm_iommu_create_mapping()' interface isn't widely used and, given that
> > > highbank doesn't support KVM, the use-cases for VFIO are pretty limited
> > > too.  
> > 
> > AFAIK Highbank doesn't have the SMMU, probably mostly for that reason.
> > I have a DT snippet for Midway, and that puts the MMIO base at ~36GB, which is not possible on Highbank.
> > So I think that the quirk is really meant and needed for Midway.  
> 
> Sorry, but I don't follow your reasoning here. The MMIO base has nothing
> to do with the quirk,

It hasn't, but Highbank has no LPAE, so couldn't possible have a device at such an address. And this is the only MMIO address I know of.

> although doing some digging it looks like your
> conclusion about this applying to Midway (ecx-2000?) is correct:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226095.html

Right, thanks for that find. Yes, Midway is the codename for the ECX-2000 SoC product.

Cheers,
Andre
 
> > > > So I don't know exactly *how* desperate you are with removing this, or if
> > > > there are other reasons than "negative diffstat", but if possible I would
> > > > like to keep it in.    
> > > 
> > > It's more that we *do* make quite a lot of changes to the arm-smmu driver
> > > and it's never tested with this quirk. If you're stepping up to run smmu
> > > tests on my queue for each release on highbank, then great, but otherwise
> > > I'd rather not carry the code for fun. The change in diffstat is minimal
> > > (we're going to need to hooks for nvidia, who broke things in a different
> > > way).  
> > 
> > I am about to set up some more sophisticated testing, and will include
> > some SMMU bits in it.  
> 
> Yes, please.
> 
> Will

Patch

diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
index 74d97a886e93..a3be8712d27f 100644
--- a/drivers/iommu/arm-smmu-impl.c
+++ b/drivers/iommu/arm-smmu-impl.c
@@ -9,45 +9,6 @@ 

 #include "arm-smmu.h"

-
-static int arm_smmu_gr0_ns(int offset)
-{
-	switch(offset) {
-	case ARM_SMMU_GR0_sCR0:
-	case ARM_SMMU_GR0_sACR:
-	case ARM_SMMU_GR0_sGFSR:
-	case ARM_SMMU_GR0_sGFSYNR0:
-	case ARM_SMMU_GR0_sGFSYNR1:
-	case ARM_SMMU_GR0_sGFSYNR2:
-		return offset + 0x400;
-	default:
-		return offset;
-	}
-}
-
-static u32 arm_smmu_read_ns(struct arm_smmu_device *smmu, int page,
-			    int offset)
-{
-	if (page == ARM_SMMU_GR0)
-		offset = arm_smmu_gr0_ns(offset);
-	return readl_relaxed(arm_smmu_page(smmu, page) + offset);
-}
-
-static void arm_smmu_write_ns(struct arm_smmu_device *smmu, int page,
-			      int offset, u32 val)
-{
-	if (page == ARM_SMMU_GR0)
-		offset = arm_smmu_gr0_ns(offset);
-	writel_relaxed(val, arm_smmu_page(smmu, page) + offset);
-}
-
-/* Since we don't care for sGFAR, we can do without 64-bit accessors */
-static const struct arm_smmu_impl calxeda_impl = {
-	.read_reg = arm_smmu_read_ns,
-	.write_reg = arm_smmu_write_ns,
-};
-
-
 struct cavium_smmu {
 	struct arm_smmu_device smmu;
 	u32 id_base;
@@ -166,10 +127,6 @@  struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
 		break;
 	}

-	if (of_property_read_bool(smmu->dev->of_node,
-				  "calxeda,smmu-secure-config-access"))
-		smmu->impl = &calxeda_impl;
-
 	if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-500"))
 		return qcom_smmu_impl_init(smmu);