[Xen-devel] [PATCH] [RFC] xen/arm: Restrict "pa_range" according to the IOMMU requirements

[Xen-devel] [PATCH] [RFC] xen/arm: Restrict "pa_range" according to the IOMMU requirements

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

There is a strict requirement for the IOMMU which wants to share
the P2M table with the CPU. The maximum supported by the IOMMU
Stage-2 input size must be greater or equal to the P2M IPA size.

So, first initialize the IOMMU and gather the requirements and
then initialize the P2M. In the P2M code, take into the account
the IOMMU requirements and restrict "pa_range" if necessary.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---

CC: Julien Grall <julien.grall@arm.com>

Why RFC?

1. Patch assumes that IPMMU support is already in.
2. Not sure for the SMMU.

If there are no objections I will craft a proper patch.
---
 xen/arch/arm/p2m.c                       | 19 +++++++++++++++++--
 xen/arch/arm/setup.c                     |  4 ++--
 xen/drivers/passthrough/arm/ipmmu-vmsa.c | 20 +++++---------------
 xen/drivers/passthrough/arm/smmu.c       | 14 +++++++-------
 4 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c171568..1262ae9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -34,7 +34,7 @@ static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
 
 #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
 
-unsigned int __read_mostly p2m_ipa_bits;
+unsigned int __read_mostly p2m_ipa_bits = 0;
 
 /* Helpers to lookup the properties of each level */
 static const paddr_t level_masks[] =
@@ -1981,10 +1981,25 @@ void __init setup_virt_paging(void)
         [7] = { 0 }  /* Invalid */
     };
 
-    unsigned int cpu;
+    unsigned int i, cpu;
     unsigned int pa_range = 0x10; /* Larger than any possible value */
     bool vmid_8_bit = false;
 
+    if ( iommu_enabled )
+    {
+        /* We expect "p2m_ipa_bits" to be updated by the IOMMU driver */
+        ASSERT(p2m_ipa_bits);
+
+        /* We need to restrict "pa_range" according to the IOMMU requirements */
+        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
+        {
+            if ( p2m_ipa_bits >= 64 - pa_range_info[i].t0sz )
+                pa_range = i;
+            else
+                break;
+        }
+    }
+
     for_each_online_cpu ( cpu )
     {
         const struct cpuinfo_arm *info = &cpu_data[cpu];
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 51a6677..01cd83d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -936,12 +936,12 @@ void __init start_xen(unsigned long boot_phys_offset,
     printk("Brought up %ld CPUs\n", (long)num_online_cpus());
     /* TODO: smp_cpus_done(); */
 
-    setup_virt_paging();
-
     rc = iommu_setup();
     if ( !iommu_enabled && rc != -ENODEV )
         panic("Couldn't configure correctly all the IOMMUs.");
 
+    setup_virt_paging();
+
     do_initcalls();
 
     /*
diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index ec543c3..0dc6351 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -526,6 +526,7 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
      * to TTBR0. Use 4KB page granule. Start page table walks at first level.
      * Always bypass stage 1 translation.
      */
+    ASSERT(p2m_ipa_bits <= IPMMU_MAX_P2M_IPA_BITS);
     tsz0 = (64 - p2m_ipa_bits) << IMTTBCR_TSZ0_SHIFT;
     ipmmu_ctx_write_root(domain, IMTTBCR, IMTTBCR_EAE | IMTTBCR_PMB |
                          IMTTBCR_SL0_LVL_1 | tsz0);
@@ -1314,23 +1315,12 @@ static __init int ipmmu_init(struct dt_device_node *node, const void *data)
         return -ENODEV;
     }
     else
-    {
         /*
-         * As 4-level translation table is not supported in IPMMU, we need
-         * to check IPA size used for P2M table beforehand to be sure it is
-         * 3-level and the IPMMU will be able to use it.
-         *
-         * TODO: First initialize the IOMMU and gather the requirements and
-         * then initialize the P2M. In the P2M code, take into the account
-         * the IOMMU requirements and restrict "pa_range" if necessary.
+         * Set max Stage-2 input size supported by the IPMMU. We expect
+         * the P2M code will take into the account the IOMMU requirements and
+         * restrict "pa_range" if necessary.
          */
-        if ( IPMMU_MAX_P2M_IPA_BITS < p2m_ipa_bits )
-        {
-            printk_once(XENLOG_ERR "ipmmu: P2M IPA size is not supported (P2M=%u IPMMU=%u)!\n",
-                        p2m_ipa_bits, IPMMU_MAX_P2M_IPA_BITS);
-            return -ENODEV;
-        }
-    }
+        p2m_ipa_bits = IPMMU_MAX_P2M_IPA_BITS;
 
     ret = ipmmu_probe(node);
     if ( ret )
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8ae986a..9b3867d 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1110,6 +1110,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 			reg = TTBCR_TG0_64K;
 
 		if (!stage1) {
+			ASSERT(p2m_ipa_bits <= smmu->s2_input_size);
 			reg |= (64 - smmu->s2_input_size) << TTBCR_T0SZ_SHIFT;
 
 			switch (smmu->s2_output_size) {
@@ -2198,13 +2199,12 @@ 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 has to match p2m_ipa_bits.  */
-	if (size < p2m_ipa_bits) {
-		dev_err(smmu->dev,
-			"P2M IPA size not supported (P2M=%u SMMU=%lu)!\n",
-			p2m_ipa_bits, size);
-		return -ENODEV;
-	}
+	/*
+	 * Xen: Set max Stage-2 input size supported by the SMMU. We expect
+	 * the P2M code will take into the account the IOMMU requirements and
+	 * restrict "pa_range" if necessary.
+	 */
+	p2m_ipa_bits = size;
 	smmu->s2_input_size = p2m_ipa_bits;
 #if 0
 	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] [RFC] xen/arm: Restrict "pa_range" according to the IOMMU requirements

[Julien Grall]

Hi Oleksandr,

Please try to get your patch on the latest Xen before sending it. So you can get 
the right people CCed. For instance, Volodymyr has not been CCed as a reviewer.

On 21/08/2019 19:17, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> There is a strict requirement for the IOMMU which wants to share
> the P2M table with the CPU. The maximum supported by the IOMMU
> Stage-2 input size must be greater or equal to the P2M IPA size.
> 
> So, first initialize the IOMMU and gather the requirements and
> then initialize the P2M. In the P2M code, take into the account
> the IOMMU requirements and restrict "pa_range" if necessary.

All the code you modify is arm64 specific. For arm32, the number of IPA bits is 
hardcoded. So if you modify p2m_ipa_bits, you would end up to misconfigure VTCR.

> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> 
> CC: Julien Grall <julien.grall@arm.com>
> 
> Why RFC?
> 
> 1. Patch assumes that IPMMU support is already in.
> 2. Not sure for the SMMU.
> 
> If there are no objections I will craft a proper patch.
> ---
>   xen/arch/arm/p2m.c                       | 19 +++++++++++++++++--
>   xen/arch/arm/setup.c                     |  4 ++--
>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 20 +++++---------------
>   xen/drivers/passthrough/arm/smmu.c       | 14 +++++++-------
>   4 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c171568..1262ae9 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -34,7 +34,7 @@ static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
>   
>   #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>   
> -unsigned int __read_mostly p2m_ipa_bits;
> +unsigned int __read_mostly p2m_ipa_bits = 0;

Any uninitialized global variables are 0 by default. But I think 0 will not be 
correct here. You are assuming all the IOMMUs can support the same number of IPA 
bits.

I am not aware if such platform exists, but this is not prevented by the SMMU 
specification. So it would be possible to have one SMMU supporting only 42-bits 
while the other would support 48-bits.

>   
>   /* Helpers to lookup the properties of each level */
>   static const paddr_t level_masks[] =
> @@ -1981,10 +1981,25 @@ void __init setup_virt_paging(void)
>           [7] = { 0 }  /* Invalid */
>       };
>   
> -    unsigned int cpu;
> +    unsigned int i, cpu;
>       unsigned int pa_range = 0x10; /* Larger than any possible value */
>       bool vmid_8_bit = false;
>   
> +    if ( iommu_enabled )
> +    {
> +        /* We expect "p2m_ipa_bits" to be updated by the IOMMU driver */

I am not entirely happy that the IOMMU driver is updating p2m_ipa_bits directly.

It makes things fairly unclear when the value of p2m_ipa_bits will be stable.

I would prefer if we provide an helper that allow restricting the size. Maybe 
p2m_restrict_ipa_bits(...).

The helper can then decide how to deal with it.

> +        ASSERT(p2m_ipa_bits);
> +
> +        /* We need to restrict "pa_range" according to the IOMMU requirements */
> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> +        {
> +            if ( p2m_ipa_bits >= 64 - pa_range_info[i].t0sz )

While I know the compiler will do the right things, this is slightly confusing 
to read. Please add 64 - pa_range_info... between parenthesis.

But I think you can just check against t0sz here. Also, it feels to me a strict 
equality would be better. If p2m_ipa_bits is neither of the value specified 
here, then most likely sharing the page tables was the wrong thing to do.

> +                pa_range = i;
> +            else
> +                break;
> +        }
> +    }
> +
>       for_each_online_cpu ( cpu )
>       {
>           const struct cpuinfo_arm *info = &cpu_data[cpu];
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 51a6677..01cd83d 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -936,12 +936,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>       printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>       /* TODO: smp_cpus_done(); */
>   
> -    setup_virt_paging();
> -
>       rc = iommu_setup();
>       if ( !iommu_enabled && rc != -ENODEV )
>           panic("Couldn't configure correctly all the IOMMUs.");
>   
> +    setup_virt_paging();

Please add a comment on top of either setup_virt_paging() (or iommu_setup()) to 
explain the dependency between the two.

> +
>       do_initcalls();
>   
>       /*
> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> index ec543c3..0dc6351 100644
> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> @@ -526,6 +526,7 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>        * to TTBR0. Use 4KB page granule. Start page table walks at first level.
>        * Always bypass stage 1 translation.
>        */
> +    ASSERT(p2m_ipa_bits <= IPMMU_MAX_P2M_IPA_BITS);

Why this ASSERT? I know that Xen is only supporting one kind of IOMMU, but you 
could imagine a platform with various of them. So this may trigger when it 
should not.

>       tsz0 = (64 - p2m_ipa_bits) << IMTTBCR_TSZ0_SHIFT;
>       ipmmu_ctx_write_root(domain, IMTTBCR, IMTTBCR_EAE | IMTTBCR_PMB |
>                            IMTTBCR_SL0_LVL_1 | tsz0);
> @@ -1314,23 +1315,12 @@ static __init int ipmmu_init(struct dt_device_node *node, const void *data)
>           return -ENODEV;
>       }
>       else
> -    {
>           /*
> -         * As 4-level translation table is not supported in IPMMU, we need
> -         * to check IPA size used for P2M table beforehand to be sure it is
> -         * 3-level and the IPMMU will be able to use it.
> -         *
> -         * TODO: First initialize the IOMMU and gather the requirements and
> -         * then initialize the P2M. In the P2M code, take into the account
> -         * the IOMMU requirements and restrict "pa_range" if necessary.
> +         * Set max Stage-2 input size supported by the IPMMU. We expect
> +         * the P2M code will take into the account the IOMMU requirements and
> +         * restrict "pa_range" if necessary.
>            */
> -        if ( IPMMU_MAX_P2M_IPA_BITS < p2m_ipa_bits )
> -        {
> -            printk_once(XENLOG_ERR "ipmmu: P2M IPA size is not supported (P2M=%u IPMMU=%u)!\n",
> -                        p2m_ipa_bits, IPMMU_MAX_P2M_IPA_BITS);
> -            return -ENODEV;
> -        }
> -    }
> +        p2m_ipa_bits = IPMMU_MAX_P2M_IPA_BITS;
>   
>       ret = ipmmu_probe(node);
>       if ( ret )
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 8ae986a..9b3867d 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1110,6 +1110,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
>   			reg = TTBCR_TG0_64K;
>   
>   		if (!stage1) {
> +			ASSERT(p2m_ipa_bits <= smmu->s2_input_size);

More importantly for the SMMU, it is possible to have a system with supporting 
different input size. So this ASSERT will trigger in that case.

>   			reg |= (64 - smmu->s2_input_size) << TTBCR_T0SZ_SHIFT;
>   
>   			switch (smmu->s2_output_size) {
> @@ -2198,13 +2199,12 @@ 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 has to match p2m_ipa_bits.  */
> -	if (size < p2m_ipa_bits) {
> -		dev_err(smmu->dev,
> -			"P2M IPA size not supported (P2M=%u SMMU=%lu)!\n",
> -			p2m_ipa_bits, size);
> -		return -ENODEV;
> -	}
> +	/*
> +	 * Xen: Set max Stage-2 input size supported by the SMMU. We expect
> +	 * the P2M code will take into the account the IOMMU requirements and
> +	 * restrict "pa_range" if necessary.
> +	 */
> +	p2m_ipa_bits = size;

See above.

>   	smmu->s2_input_size = p2m_ipa_bits;
>   #if 0
>   	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] [RFC] xen/arm: Restrict "pa_range" according to the IOMMU requirements

[Oleksandr]

On 22.08.19 15:46, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien.


Thank you for the review.


>
> Please try to get your patch on the latest Xen before sending it. So 
> you can get the right people CCed. For instance, Volodymyr has not 
> been CCed as a reviewer.

Ooh, next time will do. Sorry for that.


>
>
> On 21/08/2019 19:17, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> There is a strict requirement for the IOMMU which wants to share
>> the P2M table with the CPU. The maximum supported by the IOMMU
>> Stage-2 input size must be greater or equal to the P2M IPA size.
>>
>> So, first initialize the IOMMU and gather the requirements and
>> then initialize the P2M. In the P2M code, take into the account
>> the IOMMU requirements and restrict "pa_range" if necessary.
>
> All the code you modify is arm64 specific. For arm32, the number of 
> IPA bits is hardcoded. So if you modify p2m_ipa_bits, you would end up 
> to misconfigure VTCR.

Indeed, will guard with #ifdef CONFIG_ARM_64.


>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> Why RFC?
>>
>> 1. Patch assumes that IPMMU support is already in.
>> 2. Not sure for the SMMU.
>>
>> If there are no objections I will craft a proper patch.
>> ---
>>   xen/arch/arm/p2m.c                       | 19 +++++++++++++++++--
>>   xen/arch/arm/setup.c                     |  4 ++--
>>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 20 +++++---------------
>>   xen/drivers/passthrough/arm/smmu.c       | 14 +++++++-------
>>   4 files changed, 31 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index c171568..1262ae9 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -34,7 +34,7 @@ static unsigned int __read_mostly max_vmid = 
>> MAX_VMID_8_BIT;
>>     #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>>   -unsigned int __read_mostly p2m_ipa_bits;
>> +unsigned int __read_mostly p2m_ipa_bits = 0;
>
> Any uninitialized global variables are 0 by default. But I think 0 
> will not be correct here. You are assuming all the IOMMUs can support 
> the same number of IPA bits.

Yes, this was my assumption.


>
> I am not aware if such platform exists, but this is not prevented by 
> the SMMU specification. So it would be possible to have one SMMU 
> supporting only 42-bits while the other would support 48-bits.

What do you think would be the proper action at the moment?


>
>>     /* Helpers to lookup the properties of each level */
>>   static const paddr_t level_masks[] =
>> @@ -1981,10 +1981,25 @@ void __init setup_virt_paging(void)
>>           [7] = { 0 }  /* Invalid */
>>       };
>>   -    unsigned int cpu;
>> +    unsigned int i, cpu;
>>       unsigned int pa_range = 0x10; /* Larger than any possible value */
>>       bool vmid_8_bit = false;
>>   +    if ( iommu_enabled )
>> +    {
>> +        /* We expect "p2m_ipa_bits" to be updated by the IOMMU 
>> driver */
>
> I am not entirely happy that the IOMMU driver is updating p2m_ipa_bits 
> directly.
>
> It makes things fairly unclear when the value of p2m_ipa_bits will be 
> stable.
>
> I would prefer if we provide an helper that allow restricting the 
> size. Maybe p2m_restrict_ipa_bits(...).

Makes sense. Will implement.


>
>
> The helper can then decide how to deal with it.

OK. I am wondering, do we have cases when we shouldn't restrict the size 
(except cases when IOMMU wants wrong p2m_ipa_bits value)?




>
>> +        ASSERT(p2m_ipa_bits);
>> +
>> +        /* We need to restrict "pa_range" according to the IOMMU 
>> requirements */
>> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>> +        {
>> +            if ( p2m_ipa_bits >= 64 - pa_range_info[i].t0sz )
>
> While I know the compiler will do the right things, this is slightly 
> confusing to read. Please add 64 - pa_range_info... between parenthesis.

Will do.


>
>
> But I think you can just check against t0sz here. Also, it feels to me 
> a strict equality would be better. If p2m_ipa_bits is neither of the 
> value specified here, then most likely sharing the page tables was the 
> wrong thing to do.

I am afraid I don't completely understand why we need to check against 
t0sz. Or probably, you meant to check against pabits?


>
>
>> +                pa_range = i;
>> +            else
>> +                break;
>> +        }
>> +    }
>> +
>>       for_each_online_cpu ( cpu )
>>       {
>>           const struct cpuinfo_arm *info = &cpu_data[cpu];
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 51a6677..01cd83d 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -936,12 +936,12 @@ void __init start_xen(unsigned long 
>> boot_phys_offset,
>>       printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>>       /* TODO: smp_cpus_done(); */
>>   -    setup_virt_paging();
>> -
>>       rc = iommu_setup();
>>       if ( !iommu_enabled && rc != -ENODEV )
>>           panic("Couldn't configure correctly all the IOMMUs.");
>>   +    setup_virt_paging();
>
> Please add a comment on top of either setup_virt_paging() (or 
> iommu_setup()) to explain the dependency between the two.

Yes, will add.


>
>> +
>>       do_initcalls();
>>         /*
>> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
>> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> index ec543c3..0dc6351 100644
>> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> @@ -526,6 +526,7 @@ static int ipmmu_domain_init_context(struct 
>> ipmmu_vmsa_domain *domain)
>>        * to TTBR0. Use 4KB page granule. Start page table walks at 
>> first level.
>>        * Always bypass stage 1 translation.
>>        */
>> +    ASSERT(p2m_ipa_bits <= IPMMU_MAX_P2M_IPA_BITS);
>
> Why this ASSERT? I know that Xen is only supporting one kind of IOMMU, 
> but you could imagine a platform with various of them. So this may 
> trigger when it should not.
I didn't take into account the platform where IOMMUs could support the 
different number of IPA bits. What do you think would be the proper 
action at the moment?
I could bail out with error here (and for SMMU as well) to not allow 
creating guest domain for now...


-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] [RFC] xen/arm: Restrict "pa_range" according to the IOMMU requirements

[Julien Grall]

On 8/22/19 6:06 PM, Oleksandr wrote:
> 
> On 22.08.19 15:46, Julien Grall wrote:
>> Hi Oleksandr,
> 
> Hi Julien.

Hi,

>>
>>
>> On 21/08/2019 19:17, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> There is a strict requirement for the IOMMU which wants to share
>>> the P2M table with the CPU. The maximum supported by the IOMMU
>>> Stage-2 input size must be greater or equal to the P2M IPA size.
>>>
>>> So, first initialize the IOMMU and gather the requirements and
>>> then initialize the P2M. In the P2M code, take into the account
>>> the IOMMU requirements and restrict "pa_range" if necessary.
>>
>> All the code you modify is arm64 specific. For arm32, the number of 
>> IPA bits is hardcoded. So if you modify p2m_ipa_bits, you would end up 
>> to misconfigure VTCR.
> 
> Indeed, will guard with #ifdef CONFIG_ARM_64.

I would rather no guard any use in the IOMMU code with CONFIG_ARM_64. 
The problem is exactly the same if Arm32.

Rather than trying to limit it, I would just check that the IOMMU are 
able to support at least 40 bits. This can be done in setup_virt_paging().

> 
> 
>>
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> Why RFC?
>>>
>>> 1. Patch assumes that IPMMU support is already in.
>>> 2. Not sure for the SMMU.
>>>
>>> If there are no objections I will craft a proper patch.
>>> ---
>>>   xen/arch/arm/p2m.c                       | 19 +++++++++++++++++--
>>>   xen/arch/arm/setup.c                     |  4 ++--
>>>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 20 +++++---------------
>>>   xen/drivers/passthrough/arm/smmu.c       | 14 +++++++-------
>>>   4 files changed, 31 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index c171568..1262ae9 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -34,7 +34,7 @@ static unsigned int __read_mostly max_vmid = 
>>> MAX_VMID_8_BIT;
>>>     #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>>>   -unsigned int __read_mostly p2m_ipa_bits;
>>> +unsigned int __read_mostly p2m_ipa_bits = 0;
>>
>> Any uninitialized global variables are 0 by default. But I think 0 
>> will not be correct here. You are assuming all the IOMMUs can support 
>> the same number of IPA bits.
> 
> Yes, this was my assumption.
> 
> 
>>
>> I am not aware if such platform exists, but this is not prevented by 
>> the SMMU specification. So it would be possible to have one SMMU 
>> supporting only 42-bits while the other would support 48-bits.
> 
> What do you think would be the proper action at the moment?

We should compute the minimum of the maximum IPA bits that any IOMMU can 
support. In the example I gave, it would be 42-bits.

> 
> 
>>
>>>     /* Helpers to lookup the properties of each level */
>>>   static const paddr_t level_masks[] =
>>> @@ -1981,10 +1981,25 @@ void __init setup_virt_paging(void)
>>>           [7] = { 0 }  /* Invalid */
>>>       };
>>>   -    unsigned int cpu;
>>> +    unsigned int i, cpu;
>>>       unsigned int pa_range = 0x10; /* Larger than any possible value */
>>>       bool vmid_8_bit = false;
>>>   +    if ( iommu_enabled )
>>> +    {
>>> +        /* We expect "p2m_ipa_bits" to be updated by the IOMMU 
>>> driver */
>>
>> I am not entirely happy that the IOMMU driver is updating p2m_ipa_bits 
>> directly.
>>
>> It makes things fairly unclear when the value of p2m_ipa_bits will be 
>> stable.
>>
>> I would prefer if we provide an helper that allow restricting the 
>> size. Maybe p2m_restrict_ipa_bits(...).
> 
> Makes sense. Will implement.
> 
> 
>>
>>
>> The helper can then decide how to deal with it.
> 
> OK. I am wondering, do we have cases when we shouldn't restrict the size 
> (except cases when IOMMU wants wrong p2m_ipa_bits value)?

I don't think we should check in that helper whether the p2m_ipa_bits is 
wrong. This is up to the setup_virt_paging() to decide.

In this helper, we would only compute the minimum of the maximum. My 
point here is we can then decide whether we use p2m_ipa_bits or use a 
separate variable that is not exposed outside of p2m.c.

> 
> 
> 
> 
>>
>>> +        ASSERT(p2m_ipa_bits);
>>> +
>>> +        /* We need to restrict "pa_range" according to the IOMMU 
>>> requirements */
>>> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>>> +        {
>>> +            if ( p2m_ipa_bits >= 64 - pa_range_info[i].t0sz )
>>
>> While I know the compiler will do the right things, this is slightly 
>> confusing to read. Please add 64 - pa_range_info... between parenthesis.
> 
> Will do.
> 
> 
>>
>>
>> But I think you can just check against t0sz here. Also, it feels to me 
>> a strict equality would be better. If p2m_ipa_bits is neither of the 
>> value specified here, then most likely sharing the page tables was the 
>> wrong thing to do.
> 
> I am afraid I don't completely understand why we need to check against 
> t0sz. Or probably, you meant to check against pabits?

I meant pabits. Sorry.

Since commit, 896ebdfa3a "xen/arm: p2m: configure stage-2 page table to 
support upto 42-bit PA systems", stage-2 will always be configured with 
PA bits == IPA bits. So 64 - t0sz == pabits.

We should probably have a cleanup patch to remove t0sz and making 
clearer the correlation between the two.

>>
>>
>>> +                pa_range = i;
>>> +            else
>>> +                break;
>>> +        }
>>> +    }
>>> +
>>>       for_each_online_cpu ( cpu )
>>>       {
>>>           const struct cpuinfo_arm *info = &cpu_data[cpu];
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 51a6677..01cd83d 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -936,12 +936,12 @@ void __init start_xen(unsigned long 
>>> boot_phys_offset,
>>>       printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>>>       /* TODO: smp_cpus_done(); */
>>>   -    setup_virt_paging();
>>> -
>>>       rc = iommu_setup();
>>>       if ( !iommu_enabled && rc != -ENODEV )
>>>           panic("Couldn't configure correctly all the IOMMUs.");
>>>   +    setup_virt_paging();
>>
>> Please add a comment on top of either setup_virt_paging() (or 
>> iommu_setup()) to explain the dependency between the two.
> 
> Yes, will add.
> 
> 
>>
>>> +
>>>       do_initcalls();
>>>         /*
>>> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
>>> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>>> index ec543c3..0dc6351 100644
>>> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>>> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>>> @@ -526,6 +526,7 @@ static int ipmmu_domain_init_context(struct 
>>> ipmmu_vmsa_domain *domain)
>>>        * to TTBR0. Use 4KB page granule. Start page table walks at 
>>> first level.
>>>        * Always bypass stage 1 translation.
>>>        */
>>> +    ASSERT(p2m_ipa_bits <= IPMMU_MAX_P2M_IPA_BITS);
>>
>> Why this ASSERT? I know that Xen is only supporting one kind of IOMMU, 
>> but you could imagine a platform with various of them. So this may 
>> trigger when it should not.
> I didn't take into account the platform where IOMMUs could support the 
> different number of IPA bits. What do you think would be the proper 
> action at the moment?
> I could bail out with error here (and for SMMU as well) to not allow 
> creating guest domain for now...

See my suggestion above.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel