[PATCH v3 0/3] Set SMMU s2 input-size based on p2m tables

[PATCH v3 0/3] Set SMMU s2 input-size based on p2m tables

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

This is a fix for the issue I'm seeing on ZynqMP with missmatched
setup of the SMMU and the shared p2m page-tables with the CPU.

This implementes a global p2m_ipa_bits cap for S2 input-size as
discussed in the previous RFC.

Best regards,
Edgar

Changelog:
v2 -> v3:
* pfn -> ipa.
* Fix typos in commit msg for 3/3.

v1 -> v2:
* Use a global pfn bitsize instead of a per-domain one.

Edgar E. Iglesias (3):
  xen/arm: Re-order iommu_setup to after setup_virt_paging
  xen/arm: Add p2m_ipa_bits
  xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

 xen/arch/arm/p2m.c                 | 5 +++++
 xen/arch/arm/setup.c               | 4 ++--
 xen/drivers/passthrough/arm/smmu.c | 5 +++--
 xen/include/asm-arm/p2m.h          | 3 +++
 4 files changed, 13 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] xen/arm: Re-order iommu_setup to after setup_virt_paging

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This is needed to allow the paging setup to probe for
IPA bit sizes to be used in p2m tables prior to iommu setup.

Reviewed-by: Julien Grall <julien.grall@citrix.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4ec7c13..797673b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -800,8 +800,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     local_irq_enable();
     local_abort_enable();
 
-    iommu_setup();
-
     smp_prepare_cpus(cpus);
 
     initialize_keytable();
@@ -825,6 +823,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     setup_virt_paging();
 
+    iommu_setup();
+
     do_initcalls();
 
     /* Create initial domain 0. */
-- 
1.9.1

[PATCH v3 2/3] xen/arm: Add p2m_ipa_bits

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Export p2m_ipa_bits holding the bit size of IPA addresses used
in p2m tables.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/p2m.c        | 5 +++++
 xen/include/asm-arm/p2m.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8dfee24..377e5e6 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -27,6 +27,9 @@ static unsigned int __read_mostly p2m_root_level;
 
 #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
 
+/* Size of IPA addresses in bits.  */
+unsigned int p2m_ipa_bits;
+
 static bool_t p2m_valid(lpae_t pte)
 {
     return pte.p2m.valid;
@@ -1515,6 +1518,7 @@ void __init setup_virt_paging(void)
 
 #ifdef CONFIG_ARM_32
     printk("P2M: 40-bit IPA\n");
+    p2m_ipa_bits = 40;
     val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
     val |= VTCR_SL0(0x1); /* P2M starts at first level */
 #else /* CONFIG_ARM_64 */
@@ -1557,6 +1561,7 @@ void __init setup_virt_paging(void)
 
     p2m_root_order = pa_range_info[pa_range].root_order;
     p2m_root_level = 2 - pa_range_info[pa_range].sl0;
+    p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
 
     printk("P2M: %d-bit IPA with %d-bit PA\n",
            64 - pa_range_info[pa_range].t0sz,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 341df55..0c808f4 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -10,6 +10,9 @@
 
 #define paddr_bits PADDR_BITS
 
+/* Holds the bitsize of IPA addresses in p2m tables.  */
+extern unsigned int p2m_ipa_bits;
+
 struct domain;
 
 extern void memory_type_changed(struct domain *);
-- 
1.9.1

Re: [PATCH v3 2/3] xen/arm: Add p2m_ipa_bits

[Julien Grall]

Hi Edgar,

On 30/04/15 12:55, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Export p2m_ipa_bits holding the bit size of IPA addresses used

NIT: I think "addresses" is redundant here. IPA always has the word
"address" in it

> in p2m tables.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/arch/arm/p2m.c        | 5 +++++
>  xen/include/asm-arm/p2m.h | 3 +++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8dfee24..377e5e6 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -27,6 +27,9 @@ static unsigned int __read_mostly p2m_root_level;
>  
>  #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>  
> +/* Size of IPA addresses in bits.  */

Ditto.

> +unsigned int p2m_ipa_bits;
> +
>  static bool_t p2m_valid(lpae_t pte)
>  {
>      return pte.p2m.valid;
> @@ -1515,6 +1518,7 @@ void __init setup_virt_paging(void)
>  
>  #ifdef CONFIG_ARM_32
>      printk("P2M: 40-bit IPA\n");
> +    p2m_ipa_bits = 40;
>      val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>      val |= VTCR_SL0(0x1); /* P2M starts at first level */
>  #else /* CONFIG_ARM_64 */
> @@ -1557,6 +1561,7 @@ void __init setup_virt_paging(void)
>  
>      p2m_root_order = pa_range_info[pa_range].root_order;
>      p2m_root_level = 2 - pa_range_info[pa_range].sl0;
> +    p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
>  
>      printk("P2M: %d-bit IPA with %d-bit PA\n",
>             64 - pa_range_info[pa_range].t0sz,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 341df55..0c808f4 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -10,6 +10,9 @@
>  
>  #define paddr_bits PADDR_BITS
>  
> +/* Holds the bitsize of IPA addresses in p2m tables.  */

Same here.

s/bitsize/bit size/?

With thoses changes:

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

[PATCH v3 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

The Stage2 input-size must match what the CPU uses because
the SMMU and the CPU share page-tables.

Assert that the SMMU supports the P2M IPA bit size and use it.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/drivers/passthrough/arm/smmu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8a9b58b..5356046 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2230,8 +2230,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
 	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
 
-	/* Xen: Stage-2 input size is not restricted */
-	smmu->s2_input_size = size;
+	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
+	ASSERT(size >= p2m_ipa_bits);
+	smmu->s2_input_size = p2m_ipa_bits;
 #if 0
 	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
 #ifdef CONFIG_64BIT
-- 
1.9.1

Re: [PATCH v3 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Julien Grall]

Hi Edgar,

On 30/04/15 12:55, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> The Stage2 input-size must match what the CPU uses because
> the SMMU and the CPU share page-tables.
> 
> Assert that the SMMU supports the P2M IPA bit size and use it.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 8a9b58b..5356046 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2230,8 +2230,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
>  	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
>  
> -	/* Xen: Stage-2 input size is not restricted */
> -	smmu->s2_input_size = size;
> +	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
> +	ASSERT(size >= p2m_ipa_bits);

Sorry, I was planning to comment on this patch in the previous version
later but you send the new version quickly.

In general, ASSERT should only be used when you think that the condition
is always true. Although, we have some hardware where we know that this
condition will be hit.

On non-debug, we would get an odd error later because ASSERT is turned
into a no-op.

As this is a restriction of the driver we should print a error message
and return an appropriate error value.

The generic IOMMU driver can then decide if it's safe to continue
without the SMMU setup or panic.

FWIW, currently we use the later. I will send a patch to panic avoiding
the user to think the SMMU is correctly setup.

Regards,

-- 
Julien Grall

Re: [PATCH v3 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Edgar E. Iglesias]

On Thu, Apr 30, 2015 at 04:07:27PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 30/04/15 12:55, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > The Stage2 input-size must match what the CPU uses because
> > the SMMU and the CPU share page-tables.
> > 
> > Assert that the SMMU supports the P2M IPA bit size and use it.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  xen/drivers/passthrough/arm/smmu.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> > index 8a9b58b..5356046 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2230,8 +2230,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
> >  	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
> >  
> > -	/* Xen: Stage-2 input size is not restricted */
> > -	smmu->s2_input_size = size;
> > +	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
> > +	ASSERT(size >= p2m_ipa_bits);
> 
> Sorry, I was planning to comment on this patch in the previous version
> later but you send the new version quickly.

No worries, thanks for reviewing. I'll fix up the comments and send a new version.

> 
> In general, ASSERT should only be used when you think that the condition
> is always true. Although, we have some hardware where we know that this
> condition will be hit.
> 
> On non-debug, we would get an odd error later because ASSERT is turned
> into a no-op.
> 
> As this is a restriction of the driver we should print a error message
> and return an appropriate error value.
> 
> The generic IOMMU driver can then decide if it's safe to continue
> without the SMMU setup or panic.
> 
> FWIW, currently we use the later. I will send a patch to panic avoiding
> the user to think the SMMU is correctly setup.

This is what it looks like in my new version when starting XEN and forcing
a bad SMMU IPA size:

(XEN) P2M: 40-bit IPA with 40-bit PA
(XEN) P2M: 3 levels with order-1 root, VTCR 0x80023558
(XEN) smmu: /amba/smmu0@0xFD800000: probing hardware configuration...
(XEN) smmu: /amba/smmu0@0xFD800000: SMMUv2 with:
(XEN) smmu: /amba/smmu0@0xFD800000:     stage 2 translation
(XEN) smmu: /amba/smmu0@0xFD800000:     stream matching with 48 register groups, mask 0x7fff
(XEN) smmu: /amba/smmu0@0xFD800000:     16 context banks (0 stage-2 only)
(XEN) smmu: /amba/smmu0@0xFD800000: P2M IPA size not supported (P2M=40 SMMU=36)!
(XEN) I/O virtualisation disabled
(XEN) *** LOADING DOMAIN 0 ***

Dom0 boots fine but without IOMMU protections...

Cheers,
Edgar

Re: [PATCH v3 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Julien Grall]

Hi Edgar,

On 01/05/15 02:39, Edgar E. Iglesias wrote:
> On Thu, Apr 30, 2015 at 04:07:27PM +0100, Julien Grall wrote:
>> As this is a restriction of the driver we should print a error message
>> and return an appropriate error value.
>>
>> The generic IOMMU driver can then decide if it's safe to continue
>> without the SMMU setup or panic.
>>
>> FWIW, currently we use the later. I will send a patch to panic avoiding

Hmmm... I meant former not later here.

>> the user to think the SMMU is correctly setup.
> 
> This is what it looks like in my new version when starting XEN and forcing
> a bad SMMU IPA size:
>
> (XEN) P2M: 40-bit IPA with 40-bit PA
> (XEN) P2M: 3 levels with order-1 root, VTCR 0x80023558
> (XEN) smmu: /amba/smmu0@0xFD800000: probing hardware configuration...
> (XEN) smmu: /amba/smmu0@0xFD800000: SMMUv2 with:
> (XEN) smmu: /amba/smmu0@0xFD800000:     stage 2 translation
> (XEN) smmu: /amba/smmu0@0xFD800000:     stream matching with 48 register groups, mask 0x7fff
> (XEN) smmu: /amba/smmu0@0xFD800000:     16 context banks (0 stage-2 only)
> (XEN) smmu: /amba/smmu0@0xFD800000: P2M IPA size not supported (P2M=40 SMMU=36)!
> (XEN) I/O virtualisation disabled
> (XEN) *** LOADING DOMAIN 0 ***
> 
> Dom0 boots fine but without IOMMU protections...

I was expected this behavior. IOMMU is only important for device
passthrough. In this case, the user won't be able to assign the device
protected by this SMMU to any guest.

We currently have the same behavior as x86. Xen will continue to boot
DOM0 unless "iommu=force/required" is passed on the command line.

I think we should keep the same behavior.

Regards,

-- 
-- 
Julien Grall