Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-24 Thread Tirumalesh Chalamarla 



On 02/24/2016 11:10 AM, Mark Rutland wrote:

On Wed, Feb 24, 2016 at 03:54:48PM +, Chalamarla, Tirumalesh wrote:


On 2/24/16, 3:32 AM, "Mark Rutland"  wrote:


On Tue, Feb 23, 2016 at 03:50:21PM -0800, Tirumalesh Chalamarla wrote:

in Summary,

if i change asid-base to cavium,asid-base and still use DT for
supplying base value, is this a solution that will be accepted,


No. The property is _insufficient_, regardless of its name. This has
been pointed out more than once.

A base alone does not tell you what set of IDs it is valid to use
without risking a clash. The OS is well within its rights to allocate
_any_ ID above that base. It is not a requirement of the hardware nor
the binding that the OS allocate a contiguous set of IDs starting at the
base.

Consider:

smmu_a {
whatver,*id-base = <128>;
};

smmu_b {
whatever,*id-base = <64>;
};

In both cases, the *IDs 129+ could be allocated on both SMMUs.


Does adding a check to see if base + number of contexts supported will not 
overlap with each other
Make it acceptable.
Or do you want the size be provided from DT also?


At this point I think Robin's suggestion of giving the ThunderX SMMU a
different compatible string and treating that as a separate case
entirely is the best thing to do.

Then the only requirement is that _all_ the ThunderX SMMUs with shared
TLBs are under the control of the OS, and it can allocate IDs as it
chooses, so long as it ensures that there are no conflicts across all
the SMMUs it is in control of.


yes, resending based on the suggestions.

Mark.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-24 Thread Mark Rutland 
On Wed, Feb 24, 2016 at 03:54:48PM +, Chalamarla, Tirumalesh wrote:
> 
> On 2/24/16, 3:32 AM, "Mark Rutland"  wrote:
> 
> >On Tue, Feb 23, 2016 at 03:50:21PM -0800, Tirumalesh Chalamarla wrote:
> >> in Summary,
> >> 
> >> if i change asid-base to cavium,asid-base and still use DT for
> >> supplying base value, is this a solution that will be accepted, 
> >
> >No. The property is _insufficient_, regardless of its name. This has
> >been pointed out more than once.
> >
> >A base alone does not tell you what set of IDs it is valid to use
> >without risking a clash. The OS is well within its rights to allocate
> >_any_ ID above that base. It is not a requirement of the hardware nor
> >the binding that the OS allocate a contiguous set of IDs starting at the
> >base.
> >
> >Consider:
> >
> >smmu_a {
> > whatver,*id-base = <128>;
> >};
> >
> >smmu_b {
> > whatever,*id-base = <64>;
> >};
> >
> >In both cases, the *IDs 129+ could be allocated on both SMMUs.
> 
> Does adding a check to see if base + number of contexts supported will not 
> overlap with each other
> Make it acceptable. 
> Or do you want the size be provided from DT also?

At this point I think Robin's suggestion of giving the ThunderX SMMU a
different compatible string and treating that as a separate case
entirely is the best thing to do.

Then the only requirement is that _all_ the ThunderX SMMUs with shared
TLBs are under the control of the OS, and it can allocate IDs as it
chooses, so long as it ensures that there are no conflicts across all
the SMMUs it is in control of.

Mark.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-24 Thread Tirumalesh Chalamarla 



On 02/24/2016 05:38 AM, Robin Murphy wrote:

On 23/02/16 23:56, Tirumalesh Chalamarla wrote:



On 02/23/2016 04:19 AM, Robin Murphy wrote:

On 18/02/16 18:29, tchalama...@caviumnetworks.com wrote:

From: Tirumalesh Chalamarla 

Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
namespaces; specifically within a given node SMMU0 and SMMU1 share,
as does SMMU2 and SMMU3.

This patch tries to address these issuee by supplying asid and vmid
base from devicetree.

changes from V1:
- rebased on top of 16 bit VMID patch
- removed redundent options from DT
- insted of transform, DT now supplies starting ASID/VMID

Signed-off-by: Akula Geethasowjanya

Signed-off-by: Tirumalesh Chalamarla 
---
  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
  drivers/iommu/arm-smmu.c   | 37
+++---
  2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index bb7e569..80b8484 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -57,6 +57,14 @@ conditions.

  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.

+- asid-base :Buggy SMMUv2 implementations which doesn't satisfy
the
+ASID namespace needs, use this field to specify starting
+ASID for the SMMU.
+
+- vmid-base :Buggy SMMUv2 implementations which doesn't satisfy
the VMID
+namespace needs, use this field to specify starting VMID
+for the SMMU.
+


 From how you've described the situation, this is still just some
arbitrary number for the sake of a driver workaround and not an actual
property of the hardware. Either way there should be a proper compatible
string for this implementation, e.g. something like
"cavium,cn88xx-smmu".

Given that, you then shouldn't actually need anything else, because you
can simply keep track of how many Cavium SMMUs have been probed and
automatically generate a suitable offset, e.g. (smmu->index *
ARM_SMMU_MAX_CBS) to ensure global ASID/VMID uniqueness.


This particular solution might not be accurate in all cases. what
happens if SMMU stage 1 is used by a guest?


Even if we supported passing through stage 1 contexts, which we don't, a
malicious guest can freely ignore your arbitrary numbers in the DT, put
whatever nonsense it feels like in the TTBRs, and corrupt I/O in other
guests and/or the host.

You cannot safely pass through raw stage 1 contexts on this hardware, so
it's a non-issue.


Agreed. will re post based on the suggestions.

Robin.


  Example:

  smmu {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 003c442..dc46b9a 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -320,6 +320,9 @@ struct arm_smmu_device {
  unsigned longipa_size;
  unsigned longpa_size;

+u32asid_base;
+u32vmid_base;
+
  u32num_global_irqs;
  u32num_context_irqs;
  unsigned int*irqs;
@@ -335,8 +338,8 @@ struct arm_smmu_cfg {
  };
  #define INVALID_IRPTNDX0xff

-#define ARM_SMMU_CB_ASID(cfg)((cfg)->cbndx)
-#define ARM_SMMU_CB_VMID(cfg)((cfg)->cbndx + 1)
+#define ARM_SMMU_CB_ASID(smmu, cfg)((u16)((smmu)->asid_base +
(cfg)->cbndx))
+#define ARM_SMMU_CB_VMID(smmu, cfg)((u16)((smmu)->vmid_base +
(cfg)->cbndx))

  enum arm_smmu_domain_stage {
  ARM_SMMU_DOMAIN_S1 = 0,
@@ -576,11 +579,11 @@ static void arm_smmu_tlb_inv_context(void
*cookie)

  if (stage1) {
  base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu,
cfg->cbndx);
-writel_relaxed(ARM_SMMU_CB_ASID(cfg),
+writel_relaxed(ARM_SMMU_CB_ASID(smmu, cfg),
 base + ARM_SMMU_CB_S1_TLBIASID);
  } else {
  base = ARM_SMMU_GR0(smmu);
-writel_relaxed(ARM_SMMU_CB_VMID(cfg),
+writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg),
 base + ARM_SMMU_GR0_TLBIVMID);
  }

@@ -602,7 +605,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,

  if (!IS_ENABLED(CONFIG_64BIT) || smmu->version ==
ARM_SMMU_V1) {
  iova &= ~12UL;
-iova |= ARM_SMMU_CB_ASID(cfg);
+iova |= ARM_SMMU_CB_ASID(smmu, cfg);
  do {
  writel_relaxed(iova, reg);
  iova += granule;
@@ -610,7 +613,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,
  #ifdef CONFIG_64BIT
  } else {
  iova >>= 12;
-iova |= (u64)ARM_SMMU_CB_ASID(cfg) << 48;
+iova |= (u64)ARM_SMMU_CB_ASID(smmu, cfg) << 48;
  do {
  writeq_relaxed(iova, reg);

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-24 Thread Chalamarla, Tirumalesh 





On 2/24/16, 3:32 AM, "Mark Rutland"  wrote:

>On Tue, Feb 23, 2016 at 03:50:21PM -0800, Tirumalesh Chalamarla wrote:
>> in Summary,
>> 
>> if i change asid-base to cavium,asid-base and still use DT for
>> supplying base value, is this a solution that will be accepted, 
>
>No. The property is _insufficient_, regardless of its name. This has
>been pointed out more than once.
>
>A base alone does not tell you what set of IDs it is valid to use
>without risking a clash. The OS is well within its rights to allocate
>_any_ ID above that base. It is not a requirement of the hardware nor
>the binding that the OS allocate a contiguous set of IDs starting at the
>base.
>
>Consider:
>
>smmu_a {
>   whatver,*id-base = <128>;
>};
>
>smmu_b {
>   whatever,*id-base = <64>;
>};
>
>In both cases, the *IDs 129+ could be allocated on both SMMUs.

Does adding a check to see if base + number of contexts supported will not 
overlap with each other
Make it acceptable. 
Or do you want the size be provided from DT also?

Thanks,
Tirumalesh. 
>
>Mark.
>
>> of course i will do range check to see we are not supplying 16bit VMID
>> for 8 bit systems even though the property now indicates Cavium only.
>> 
>> Thanks,
>> Tirumalesh.
>> 
>> On 02/23/2016 04:26 AM, Mark Rutland wrote:
>> >On Thu, Feb 18, 2016 at 10:29:18AM -0800, tchalama...@caviumnetworks.com 
>> >wrote:
>> >>From: Tirumalesh Chalamarla 
>> >>
>> >>Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
>> >>namespaces; specifically within a given node SMMU0 and SMMU1 share,
>> >>as does SMMU2 and SMMU3.
>> >>
>> >>This patch tries to address these issuee by supplying asid and vmid
>> >>base from devicetree.
>> >>
>> >>changes from V1:
>> >>   - rebased on top of 16 bit VMID patch
>> >>   - removed redundent options from DT
>> >>   - insted of transform, DT now supplies starting ASID/VMID
>> >>
>> >>Signed-off-by: Akula Geethasowjanya 
>> >>
>> >>Signed-off-by: Tirumalesh Chalamarla 
>> >>---
>> >>  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
>> >>  drivers/iommu/arm-smmu.c   | 37 
>> >> +++---
>> >>  2 files changed, 34 insertions(+), 11 deletions(-)
>> >>
>> >>diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
>> >>b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> >>index bb7e569..80b8484 100644
>> >>--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> >>+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> >>@@ -57,6 +57,14 @@ conditions.
>> >>
>> >>  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.
>> >>
>> >>+- asid-base : Buggy SMMUv2 implementations which doesn't satisfy the
>> >>+  ASID namespace needs, use this field to specify starting
>> >>+  ASID for the SMMU.
>> >>+
>> >>+- vmid-base : Buggy SMMUv2 implementations which doesn't satisfy the 
>> >>VMID
>> >>+  namespace needs, use this field to specify starting VMID
>> >>+  for the SMMU.
>> >
>> >As has been pointed out, these are not strictly properties of the
>> >hardware, and are insufficient to aovid the issue in general (adding an
>> >arbitrary base does not enforce IDs fall within a particular range).
>> >
>> >So NAK for *-base properties alone.
>> >
>> >>+  if (of_property_read_u32(dev->of_node, "#asid-base",
>> >>+   >asid_base)) {
>> >>+  smmu->asid_base = 0;
>> >>+  }
>> >>+
>> >>+  if (of_property_read_u32(dev->of_node, "#vmid-base",
>> >>+   >vmid_base)) {
>> >>+  smmu->vmid_base = 1;
>> >>+  }
>> >
>> >These do not match the documentation above.
>> >
>> >Mark.
>> >
>> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-24 Thread Robin Murphy 

On 23/02/16 23:56, Tirumalesh Chalamarla wrote:



On 02/23/2016 04:19 AM, Robin Murphy wrote:

On 18/02/16 18:29, tchalama...@caviumnetworks.com wrote:

From: Tirumalesh Chalamarla 

Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
namespaces; specifically within a given node SMMU0 and SMMU1 share,
as does SMMU2 and SMMU3.

This patch tries to address these issuee by supplying asid and vmid
base from devicetree.

changes from V1:
- rebased on top of 16 bit VMID patch
- removed redundent options from DT
- insted of transform, DT now supplies starting ASID/VMID

Signed-off-by: Akula Geethasowjanya

Signed-off-by: Tirumalesh Chalamarla 
---
  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
  drivers/iommu/arm-smmu.c   | 37
+++---
  2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index bb7e569..80b8484 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -57,6 +57,14 @@ conditions.

  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.

+- asid-base :Buggy SMMUv2 implementations which doesn't satisfy the
+ASID namespace needs, use this field to specify starting
+ASID for the SMMU.
+
+- vmid-base :Buggy SMMUv2 implementations which doesn't satisfy
the VMID
+namespace needs, use this field to specify starting VMID
+for the SMMU.
+


 From how you've described the situation, this is still just some
arbitrary number for the sake of a driver workaround and not an actual
property of the hardware. Either way there should be a proper compatible
string for this implementation, e.g. something like "cavium,cn88xx-smmu".

Given that, you then shouldn't actually need anything else, because you
can simply keep track of how many Cavium SMMUs have been probed and
automatically generate a suitable offset, e.g. (smmu->index *
ARM_SMMU_MAX_CBS) to ensure global ASID/VMID uniqueness.


This particular solution might not be accurate in all cases. what
happens if SMMU stage 1 is used by a guest?


Even if we supported passing through stage 1 contexts, which we don't, a 
malicious guest can freely ignore your arbitrary numbers in the DT, put 
whatever nonsense it feels like in the TTBRs, and corrupt I/O in other 
guests and/or the host.


You cannot safely pass through raw stage 1 contexts on this hardware, so 
it's a non-issue.


Robin.


  Example:

  smmu {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 003c442..dc46b9a 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -320,6 +320,9 @@ struct arm_smmu_device {
  unsigned longipa_size;
  unsigned longpa_size;

+u32asid_base;
+u32vmid_base;
+
  u32num_global_irqs;
  u32num_context_irqs;
  unsigned int*irqs;
@@ -335,8 +338,8 @@ struct arm_smmu_cfg {
  };
  #define INVALID_IRPTNDX0xff

-#define ARM_SMMU_CB_ASID(cfg)((cfg)->cbndx)
-#define ARM_SMMU_CB_VMID(cfg)((cfg)->cbndx + 1)
+#define ARM_SMMU_CB_ASID(smmu, cfg)((u16)((smmu)->asid_base +
(cfg)->cbndx))
+#define ARM_SMMU_CB_VMID(smmu, cfg)((u16)((smmu)->vmid_base +
(cfg)->cbndx))

  enum arm_smmu_domain_stage {
  ARM_SMMU_DOMAIN_S1 = 0,
@@ -576,11 +579,11 @@ static void arm_smmu_tlb_inv_context(void *cookie)

  if (stage1) {
  base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
-writel_relaxed(ARM_SMMU_CB_ASID(cfg),
+writel_relaxed(ARM_SMMU_CB_ASID(smmu, cfg),
 base + ARM_SMMU_CB_S1_TLBIASID);
  } else {
  base = ARM_SMMU_GR0(smmu);
-writel_relaxed(ARM_SMMU_CB_VMID(cfg),
+writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg),
 base + ARM_SMMU_GR0_TLBIVMID);
  }

@@ -602,7 +605,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,

  if (!IS_ENABLED(CONFIG_64BIT) || smmu->version ==
ARM_SMMU_V1) {
  iova &= ~12UL;
-iova |= ARM_SMMU_CB_ASID(cfg);
+iova |= ARM_SMMU_CB_ASID(smmu, cfg);
  do {
  writel_relaxed(iova, reg);
  iova += granule;
@@ -610,7 +613,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,
  #ifdef CONFIG_64BIT
  } else {
  iova >>= 12;
-iova |= (u64)ARM_SMMU_CB_ASID(cfg) << 48;
+iova |= (u64)ARM_SMMU_CB_ASID(smmu, cfg) << 48;
  do {
  writeq_relaxed(iova, reg);
  iova += granule >> 12;
@@ -630,7 +633,7 @@ static void

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-24 Thread Mark Rutland 
On Tue, Feb 23, 2016 at 03:50:21PM -0800, Tirumalesh Chalamarla wrote:
> in Summary,
> 
> if i change asid-base to cavium,asid-base and still use DT for
> supplying base value, is this a solution that will be accepted, 

No. The property is _insufficient_, regardless of its name. This has
been pointed out more than once.

A base alone does not tell you what set of IDs it is valid to use
without risking a clash. The OS is well within its rights to allocate
_any_ ID above that base. It is not a requirement of the hardware nor
the binding that the OS allocate a contiguous set of IDs starting at the
base.

Consider:

smmu_a {
whatver,*id-base = <128>;
};

smmu_b {
whatever,*id-base = <64>;
};

In both cases, the *IDs 129+ could be allocated on both SMMUs.

Mark.

> of course i will do range check to see we are not supplying 16bit VMID
> for 8 bit systems even though the property now indicates Cavium only.
> 
> Thanks,
> Tirumalesh.
> 
> On 02/23/2016 04:26 AM, Mark Rutland wrote:
> >On Thu, Feb 18, 2016 at 10:29:18AM -0800, tchalama...@caviumnetworks.com 
> >wrote:
> >>From: Tirumalesh Chalamarla 
> >>
> >>Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
> >>namespaces; specifically within a given node SMMU0 and SMMU1 share,
> >>as does SMMU2 and SMMU3.
> >>
> >>This patch tries to address these issuee by supplying asid and vmid
> >>base from devicetree.
> >>
> >>changes from V1:
> >>- rebased on top of 16 bit VMID patch
> >>- removed redundent options from DT
> >>- insted of transform, DT now supplies starting ASID/VMID
> >>
> >>Signed-off-by: Akula Geethasowjanya 
> >>
> >>Signed-off-by: Tirumalesh Chalamarla 
> >>---
> >>  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
> >>  drivers/iommu/arm-smmu.c   | 37 
> >> +++---
> >>  2 files changed, 34 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> >>b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >>index bb7e569..80b8484 100644
> >>--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >>+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >>@@ -57,6 +57,14 @@ conditions.
> >>
> >>  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.
> >>
> >>+- asid-base :  Buggy SMMUv2 implementations which doesn't satisfy the
> >>+   ASID namespace needs, use this field to specify starting
> >>+   ASID for the SMMU.
> >>+
> >>+- vmid-base :  Buggy SMMUv2 implementations which doesn't satisfy the 
> >>VMID
> >>+   namespace needs, use this field to specify starting VMID
> >>+   for the SMMU.
> >
> >As has been pointed out, these are not strictly properties of the
> >hardware, and are insufficient to aovid the issue in general (adding an
> >arbitrary base does not enforce IDs fall within a particular range).
> >
> >So NAK for *-base properties alone.
> >
> >>+   if (of_property_read_u32(dev->of_node, "#asid-base",
> >>+>asid_base)) {
> >>+   smmu->asid_base = 0;
> >>+   }
> >>+
> >>+   if (of_property_read_u32(dev->of_node, "#vmid-base",
> >>+>vmid_base)) {
> >>+   smmu->vmid_base = 1;
> >>+   }
> >
> >These do not match the documentation above.
> >
> >Mark.
> >
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-23 Thread Tirumalesh Chalamarla 



On 02/23/2016 04:19 AM, Robin Murphy wrote:

On 18/02/16 18:29, tchalama...@caviumnetworks.com wrote:

From: Tirumalesh Chalamarla 

Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
namespaces; specifically within a given node SMMU0 and SMMU1 share,
as does SMMU2 and SMMU3.

This patch tries to address these issuee by supplying asid and vmid
base from devicetree.

changes from V1:
- rebased on top of 16 bit VMID patch
- removed redundent options from DT
- insted of transform, DT now supplies starting ASID/VMID

Signed-off-by: Akula Geethasowjanya

Signed-off-by: Tirumalesh Chalamarla 
---
  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
  drivers/iommu/arm-smmu.c   | 37
+++---
  2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index bb7e569..80b8484 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -57,6 +57,14 @@ conditions.

  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.

+- asid-base :Buggy SMMUv2 implementations which doesn't satisfy the
+ASID namespace needs, use this field to specify starting
+ASID for the SMMU.
+
+- vmid-base :Buggy SMMUv2 implementations which doesn't satisfy
the VMID
+namespace needs, use this field to specify starting VMID
+for the SMMU.
+


 From how you've described the situation, this is still just some
arbitrary number for the sake of a driver workaround and not an actual
property of the hardware. Either way there should be a proper compatible
string for this implementation, e.g. something like "cavium,cn88xx-smmu".

Given that, you then shouldn't actually need anything else, because you
can simply keep track of how many Cavium SMMUs have been probed and
automatically generate a suitable offset, e.g. (smmu->index *
ARM_SMMU_MAX_CBS) to ensure global ASID/VMID uniqueness.


This particular solution might not be accurate in all cases. what 
happens if SMMU stage 1 is used by a guest?


Robin.


  Example:

  smmu {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 003c442..dc46b9a 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -320,6 +320,9 @@ struct arm_smmu_device {
  unsigned longipa_size;
  unsigned longpa_size;

+u32asid_base;
+u32vmid_base;
+
  u32num_global_irqs;
  u32num_context_irqs;
  unsigned int*irqs;
@@ -335,8 +338,8 @@ struct arm_smmu_cfg {
  };
  #define INVALID_IRPTNDX0xff

-#define ARM_SMMU_CB_ASID(cfg)((cfg)->cbndx)
-#define ARM_SMMU_CB_VMID(cfg)((cfg)->cbndx + 1)
+#define ARM_SMMU_CB_ASID(smmu, cfg)((u16)((smmu)->asid_base +
(cfg)->cbndx))
+#define ARM_SMMU_CB_VMID(smmu, cfg)((u16)((smmu)->vmid_base +
(cfg)->cbndx))

  enum arm_smmu_domain_stage {
  ARM_SMMU_DOMAIN_S1 = 0,
@@ -576,11 +579,11 @@ static void arm_smmu_tlb_inv_context(void *cookie)

  if (stage1) {
  base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
-writel_relaxed(ARM_SMMU_CB_ASID(cfg),
+writel_relaxed(ARM_SMMU_CB_ASID(smmu, cfg),
 base + ARM_SMMU_CB_S1_TLBIASID);
  } else {
  base = ARM_SMMU_GR0(smmu);
-writel_relaxed(ARM_SMMU_CB_VMID(cfg),
+writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg),
 base + ARM_SMMU_GR0_TLBIVMID);
  }

@@ -602,7 +605,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,

  if (!IS_ENABLED(CONFIG_64BIT) || smmu->version ==
ARM_SMMU_V1) {
  iova &= ~12UL;
-iova |= ARM_SMMU_CB_ASID(cfg);
+iova |= ARM_SMMU_CB_ASID(smmu, cfg);
  do {
  writel_relaxed(iova, reg);
  iova += granule;
@@ -610,7 +613,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,
  #ifdef CONFIG_64BIT
  } else {
  iova >>= 12;
-iova |= (u64)ARM_SMMU_CB_ASID(cfg) << 48;
+iova |= (u64)ARM_SMMU_CB_ASID(smmu, cfg) << 48;
  do {
  writeq_relaxed(iova, reg);
  iova += granule >> 12;
@@ -630,7 +633,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned
long iova, size_t size,
  #endif
  } else {
  reg = ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_TLBIVMID;
-writel_relaxed(ARM_SMMU_CB_VMID(cfg), reg);
+writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg), reg);
  }
  }

@@ -744,7 +747,7 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain,
  #endif
  /* if 16bit VMID required set

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-23 Thread Tirumalesh Chalamarla 

in Summary,

if i change asid-base to cavium,asid-base and still use DT for supplying 
base value, is this a solution that will be accepted, of course i will 
do range check to see we are not supplying 16bit VMID for 8 bit systems 
even though the property now indicates Cavium only.


Thanks,
Tirumalesh.

On 02/23/2016 04:26 AM, Mark Rutland wrote:

On Thu, Feb 18, 2016 at 10:29:18AM -0800, tchalama...@caviumnetworks.com wrote:

From: Tirumalesh Chalamarla 

Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
namespaces; specifically within a given node SMMU0 and SMMU1 share,
as does SMMU2 and SMMU3.

This patch tries to address these issuee by supplying asid and vmid
base from devicetree.

changes from V1:
- rebased on top of 16 bit VMID patch
- removed redundent options from DT
- insted of transform, DT now supplies starting ASID/VMID

Signed-off-by: Akula Geethasowjanya 
Signed-off-by: Tirumalesh Chalamarla 
---
  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
  drivers/iommu/arm-smmu.c   | 37 +++---
  2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index bb7e569..80b8484 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -57,6 +57,14 @@ conditions.

  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.

+- asid-base :  Buggy SMMUv2 implementations which doesn't satisfy the
+   ASID namespace needs, use this field to specify starting
+   ASID for the SMMU.
+
+- vmid-base :  Buggy SMMUv2 implementations which doesn't satisfy the VMID
+   namespace needs, use this field to specify starting VMID
+   for the SMMU.


As has been pointed out, these are not strictly properties of the
hardware, and are insufficient to aovid the issue in general (adding an
arbitrary base does not enforce IDs fall within a particular range).

So NAK for *-base properties alone.


+   if (of_property_read_u32(dev->of_node, "#asid-base",
+>asid_base)) {
+   smmu->asid_base = 0;
+   }
+
+   if (of_property_read_u32(dev->of_node, "#vmid-base",
+>vmid_base)) {
+   smmu->vmid_base = 1;
+   }


These do not match the documentation above.

Mark.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

2016-02-18 Thread tchalamarla 
From: Tirumalesh Chalamarla 

Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
namespaces; specifically within a given node SMMU0 and SMMU1 share,
as does SMMU2 and SMMU3.

This patch tries to address these issuee by supplying asid and vmid
base from devicetree.

changes from V1:
- rebased on top of 16 bit VMID patch
- removed redundent options from DT
- insted of transform, DT now supplies starting ASID/VMID

Signed-off-by: Akula Geethasowjanya 
Signed-off-by: Tirumalesh Chalamarla 
---
 .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
 drivers/iommu/arm-smmu.c   | 37 +++---
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index bb7e569..80b8484 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -57,6 +57,14 @@ conditions.
 
 - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.
 
+- asid-base :  Buggy SMMUv2 implementations which doesn't satisfy the
+   ASID namespace needs, use this field to specify starting
+   ASID for the SMMU.
+
+- vmid-base :  Buggy SMMUv2 implementations which doesn't satisfy the VMID
+   namespace needs, use this field to specify starting VMID
+   for the SMMU.
+
 Example:
 
 smmu {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 003c442..dc46b9a 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -320,6 +320,9 @@ struct arm_smmu_device {
unsigned long   ipa_size;
unsigned long   pa_size;
 
+   u32 asid_base;
+   u32 vmid_base;
+
u32 num_global_irqs;
u32 num_context_irqs;
unsigned int*irqs;
@@ -335,8 +338,8 @@ struct arm_smmu_cfg {
 };
 #define INVALID_IRPTNDX0xff
 
-#define ARM_SMMU_CB_ASID(cfg)  ((cfg)->cbndx)
-#define ARM_SMMU_CB_VMID(cfg)  ((cfg)->cbndx + 1)
+#define ARM_SMMU_CB_ASID(smmu, cfg)((u16)((smmu)->asid_base + 
(cfg)->cbndx))
+#define ARM_SMMU_CB_VMID(smmu, cfg)((u16)((smmu)->vmid_base + 
(cfg)->cbndx))
 
 enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_S1 = 0,
@@ -576,11 +579,11 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 
if (stage1) {
base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
-   writel_relaxed(ARM_SMMU_CB_ASID(cfg),
+   writel_relaxed(ARM_SMMU_CB_ASID(smmu, cfg),
   base + ARM_SMMU_CB_S1_TLBIASID);
} else {
base = ARM_SMMU_GR0(smmu);
-   writel_relaxed(ARM_SMMU_CB_VMID(cfg),
+   writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg),
   base + ARM_SMMU_GR0_TLBIVMID);
}
 
@@ -602,7 +605,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long 
iova, size_t size,
 
if (!IS_ENABLED(CONFIG_64BIT) || smmu->version == ARM_SMMU_V1) {
iova &= ~12UL;
-   iova |= ARM_SMMU_CB_ASID(cfg);
+   iova |= ARM_SMMU_CB_ASID(smmu, cfg);
do {
writel_relaxed(iova, reg);
iova += granule;
@@ -610,7 +613,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long 
iova, size_t size,
 #ifdef CONFIG_64BIT
} else {
iova >>= 12;
-   iova |= (u64)ARM_SMMU_CB_ASID(cfg) << 48;
+   iova |= (u64)ARM_SMMU_CB_ASID(smmu, cfg) << 48;
do {
writeq_relaxed(iova, reg);
iova += granule >> 12;
@@ -630,7 +633,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long 
iova, size_t size,
 #endif
} else {
reg = ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_TLBIVMID;
-   writel_relaxed(ARM_SMMU_CB_VMID(cfg), reg);
+   writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg), reg);
}
 }
 
@@ -744,7 +747,7 @@ static void arm_smmu_init_context_bank(struct 
arm_smmu_domain *smmu_domain,
 #endif
/* if 16bit VMID required set VMID in CBA2R */
if (smmu->options & ARM_SMMU_OPT_ENABLE_VMID16)
-   reg |= ARM_SMMU_CB_VMID(cfg) << CBA2R_VMID_SHIFT;
+   reg |= ARM_SMMU_CB_VMID(smmu, cfg) << CBA2R_VMID_SHIFT;
 
writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
}
@@ -763,7 +766,7 @@ static void