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 IOMMU's Stage-2 input size must be equal
to the P2M IPA size. It is not a problem when the IOMMU can support
all values the CPU supports. In that case, the IOMMU driver would just
use any "p2m_ipa_bits" value as is. But, there are cases when not.

In order to make P2M sharing possible on the platforms which
IPMMUs have a limitation in maximum Stage-2 input size introduce
the following logic.

First initialize the IOMMU subsystem and gather requirements regarding
the maximum IPA bits supported by each IOMMU device to figure out
the minimum value among them. In the P2M code, take into the account
the IOMMU requirements and choose suitable "pa_range" according
to the restricted "p2m_ipa_bits".

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
Still RFC:

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

Changes since RFC V1 [1]:
   - Don't update p2m_ipa_bits by the IOMMU drivers directly,
     introduce p2m_restrict_ipa_bits()
   - Clarify patch subject/description
   - Add more comments to code
   - Check for equivalent "pabits" in setup_virt_paging()
   - Remove ASSERTs from the SMMU and IPMMU drivers

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg02078.html
---
 xen/arch/arm/p2m.c                       | 33 ++++++++++++++++++++++++++++++--
 xen/arch/arm/setup.c                     | 11 +++++++++--
 xen/drivers/passthrough/arm/ipmmu-vmsa.c | 19 ++++--------------
 xen/drivers/passthrough/arm/smmu.c       | 16 ++++++++--------
 xen/include/asm-arm/p2m.h                |  8 ++++++++
 5 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2374e92..f742d9c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -34,7 +34,8 @@ 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;
+/* Larger than any possible value */
+unsigned int __read_mostly p2m_ipa_bits = 64;
 
 /* Helpers to lookup the properties of each level */
 static const paddr_t level_masks[] =
@@ -1912,6 +1913,16 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     return page;
 }
 
+void __init p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits)
+{
+    /*
+     * Calculate the minimum of the maximum IPA bits that any IOMMU
+     * can support.
+     */
+    if ( iommu_ipa_bits < p2m_ipa_bits )
+        p2m_ipa_bits = iommu_ipa_bits;
+}
+
 /* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
 static uint32_t __read_mostly vtcr;
 
@@ -1966,10 +1977,28 @@ 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 )
+    {
+        /*
+         * Choose suitable "pa_range" according to the IOMMU requirements
+         * (restricted "p2m_ipa_bits" value).
+         * As P2M table is always configured with IPA bits == PA bits,
+         * check for equivalent "pabits" and store it's index.
+         */
+        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
+        {
+            if ( p2m_ipa_bits == pa_range_info[i].pabits )
+            {
+                pa_range = i;
+                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..413f3e6 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -936,12 +936,19 @@ 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();
-
+    /*
+     * The IOMMU subsystem must be initialized before P2M as we need to gather
+     * requirements regarding the maximum IPA bits supported by each IOMMU
+     * device to figure out the minimum value among them. The P2M code will
+     * choose suitable "pa_range" according to the restricted "p2m_ipa_bits"
+     * value.
+     */
     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..d2e36a4 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -1314,23 +1314,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 maximum Stage-2 input size supported by the IPMMU. We expect
+         * the P2M code will take into the account the IOMMU requirements and
+         * choose suitable "pa_range".
          */
-        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_restrict_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..2b10c6e 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2198,14 +2198,14 @@ 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;
-	}
-	smmu->s2_input_size = p2m_ipa_bits;
+	/*
+	 * Xen:
+	 * Set maximum Stage-2 input size supported by the SMMU. We expect
+	 * the P2M code will take into the account the IOMMU requirements and
+	 * choose suitable "pa_range".
+	 */
+	p2m_restrict_ipa_bits(size);
+	smmu->s2_input_size = size;
 #if 0
 	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
 #ifdef CONFIG_64BIT
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f970c53..cdcf83a 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -165,6 +165,14 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
     /* Not supported on ARM. */
 }
 
+/*
+ * Helper to restrict "p2m_ipa_bits" according the IOMMU requirements.
+ *
+ * Each IOMMU driver should report the maximum IPA bits (Stage-2 input size)
+ * it can support.
+ */
+void p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits);
+
 /* Second stage paging setup, to be called on all CPUs */
 void setup_virt_paging(void);
 
-- 
2.7.4


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

Hi Oleksandr,

On 8/23/19 8:34 PM, 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 IOMMU's Stage-2 input size must be equal
> to the P2M IPA size. It is not a problem when the IOMMU can support
> all values the CPU supports. In that case, the IOMMU driver would just
> use any "p2m_ipa_bits" value as is. But, there are cases when not.
> 
> In order to make P2M sharing possible on the platforms which
> IPMMUs have a limitation in maximum Stage-2 input size introduce
> the following logic.
> 
> First initialize the IOMMU subsystem and gather requirements regarding
> the maximum IPA bits supported by each IOMMU device to figure out
> the minimum value among them. In the P2M code, take into the account
> the IOMMU requirements and choose suitable "pa_range" according
> to the restricted "p2m_ipa_bits".

As I pointed in the previous version, 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.

In other words, for Arm32, you need to check p2m_ipa_bits is at least 
40-bits before overriding it.

> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
> Still RFC:
> 
> 1. Patch assumes that IPMMU support is already in.
> 2. Not checked for the SMMU.
> 
> Changes since RFC V1 [1]:
>     - Don't update p2m_ipa_bits by the IOMMU drivers directly,
>       introduce p2m_restrict_ipa_bits()
>     - Clarify patch subject/description
>     - Add more comments to code
>     - Check for equivalent "pabits" in setup_virt_paging()
>     - Remove ASSERTs from the SMMU and IPMMU drivers
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg02078.html
> ---
>   xen/arch/arm/p2m.c                       | 33 ++++++++++++++++++++++++++++++--
>   xen/arch/arm/setup.c                     | 11 +++++++++--
>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 19 ++++--------------
>   xen/drivers/passthrough/arm/smmu.c       | 16 ++++++++--------
>   xen/include/asm-arm/p2m.h                |  8 ++++++++
>   5 files changed, 60 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2374e92..f742d9c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -34,7 +34,8 @@ 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;
> +/* Larger than any possible value */

I think it would be worth explaining that this is required so the number 
of P2M bits can be restricted by external entity (e.g IOMMU).

> +unsigned int __read_mostly p2m_ipa_bits = 64;
>   
>   /* Helpers to lookup the properties of each level */
>   static const paddr_t level_masks[] =
> @@ -1912,6 +1913,16 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>       return page;
>   }
>   
> +void __init p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits)

The name of the function is quite generic as most of the code in it. So 
can we avoid use the term IOMMU in it?

> +{
> +    /*
> +     * Calculate the minimum of the maximum IPA bits that any IOMMU
> +     * can support.
> +     */
> +    if ( iommu_ipa_bits < p2m_ipa_bits )
> +        p2m_ipa_bits = iommu_ipa_bits;
> +}
> +
>   /* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
>   static uint32_t __read_mostly vtcr;
>   
> @@ -1966,10 +1977,28 @@ 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 )

Could we make this IOMMU-agnostic? The main reason to convert from 
p2m_ipa_bits to pa_range is to cater the rest of the code.

But we could rework the code to do the computation with p2m_ipa_bits and 
then look-up for the pa_range. In all honesty, I think we can completely 
avoid pa_range but this is probably going to require more a bit more work.

> +    {
> +        /*
> +         * Choose suitable "pa_range" according to the IOMMU requirements
> +         * (restricted "p2m_ipa_bits" value).
> +         * As P2M table is always configured with IPA bits == PA bits,
> +         * check for equivalent "pabits" and store it's index.
> +         */
> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> +        {
> +            if ( p2m_ipa_bits == pa_range_info[i].pabits )
> +            {
> +                pa_range = i;
> +                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..413f3e6 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -936,12 +936,19 @@ 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();
> -
> +    /*
> +     * The IOMMU subsystem must be initialized before P2M as we need to gather
> +     * requirements regarding the maximum IPA bits supported by each IOMMU
> +     * device to figure out the minimum value among them. The P2M code will
> +     * choose suitable "pa_range" according to the restricted "p2m_ipa_bits"
> +     * value.
> +     */

This is a bit too verbose, implementation details are not necessary here 
and increasing the risk to have bit rot comment. So how about:

"The IOMMU subsystem must be initialized before the P2M as we need to 
gather requirements regarding the maximum IPA bit supported by each IOMMU."?

>       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..d2e36a4 100644
> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> @@ -1314,23 +1314,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 maximum Stage-2 input size supported by the IPMMU. We expect
> +         * the P2M code will take into the account the IOMMU requirements and
> +         * choose suitable "pa_range".

I would drop the last sentence.

>            */
> -        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_restrict_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..2b10c6e 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2198,14 +2198,14 @@ 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;
> -	}
> -	smmu->s2_input_size = p2m_ipa_bits;
> +	/*
> +	 * Xen:
> +	 * Set maximum Stage-2 input size supported by the SMMU. We expect
> +	 * the P2M code will take into the account the IOMMU requirements and
> +	 * choose suitable "pa_range".

Same here.

> +	 */
> +	p2m_restrict_ipa_bits(size);
> +	smmu->s2_input_size = size;
>   #if 0
>   	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
>   #ifdef CONFIG_64BIT
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f970c53..cdcf83a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -165,6 +165,14 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>       /* Not supported on ARM. */
>   }
>   
> +/*
> + * Helper to restrict "p2m_ipa_bits" according the IOMMU requirements.
> + *
> + * Each IOMMU driver should report the maximum IPA bits (Stage-2 input size)
> + * it can support.
> + */
> +void p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits);
> +
>   /* Second stage paging setup, to be called on all CPUs */
>   void setup_virt_paging(void);
>   
> 

Cheers,

-- 
Julien Grall

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

On 10.09.19 18:11, Julien Grall wrote:
> Hi Oleksandr,

Hi, Julien


>
> On 8/23/19 8:34 PM, 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 IOMMU's Stage-2 input size must be equal
>> to the P2M IPA size. It is not a problem when the IOMMU can support
>> all values the CPU supports. In that case, the IOMMU driver would just
>> use any "p2m_ipa_bits" value as is. But, there are cases when not.
>>
>> In order to make P2M sharing possible on the platforms which
>> IPMMUs have a limitation in maximum Stage-2 input size introduce
>> the following logic.
>>
>> First initialize the IOMMU subsystem and gather requirements regarding
>> the maximum IPA bits supported by each IOMMU device to figure out
>> the minimum value among them. In the P2M code, take into the account
>> the IOMMU requirements and choose suitable "pa_range" according
>> to the restricted "p2m_ipa_bits".
>
> As I pointed in the previous version, 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.
> In other words, for Arm32, you need to check p2m_ipa_bits is at least 
> 40-bits before overriding it.

But, all modifications with p2m_ipa_bits are done before 
setup_virt_paging(), where, actually, the p2m_ipa_bits is hard-coded to 
40 bits. How can we end up misconfiguring VTCR for ARM32? Or I really 
missed something?


>
>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Still RFC:
>>
>> 1. Patch assumes that IPMMU support is already in.
>> 2. Not checked for the SMMU.
>>
>> Changes since RFC V1 [1]:
>>     - Don't update p2m_ipa_bits by the IOMMU drivers directly,
>>       introduce p2m_restrict_ipa_bits()
>>     - Clarify patch subject/description
>>     - Add more comments to code
>>     - Check for equivalent "pabits" in setup_virt_paging()
>>     - Remove ASSERTs from the SMMU and IPMMU drivers
>>
>> [1] 
>> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg02078.html
>> ---
>>   xen/arch/arm/p2m.c                       | 33 
>> ++++++++++++++++++++++++++++++--
>>   xen/arch/arm/setup.c                     | 11 +++++++++--
>>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 19 ++++--------------
>>   xen/drivers/passthrough/arm/smmu.c       | 16 ++++++++--------
>>   xen/include/asm-arm/p2m.h                |  8 ++++++++
>>   5 files changed, 60 insertions(+), 27 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 2374e92..f742d9c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -34,7 +34,8 @@ 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;
>> +/* Larger than any possible value */
>
> I think it would be worth explaining that this is required so the 
> number of P2M bits can be restricted by external entity (e.g IOMMU).

ok, will add explanation


>
>
>> +unsigned int __read_mostly p2m_ipa_bits = 64;
>>     /* Helpers to lookup the properties of each level */
>>   static const paddr_t level_masks[] =
>> @@ -1912,6 +1913,16 @@ struct page_info *get_page_from_gva(struct 
>> vcpu *v, vaddr_t va,
>>       return page;
>>   }
>>   +void __init p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits)
>
> The name of the function is quite generic as most of the code in it. 
> So can we avoid use the term IOMMU in it?

yes, will do


>
>
>> +{
>> +    /*
>> +     * Calculate the minimum of the maximum IPA bits that any IOMMU
>> +     * can support.
>> +     */
>> +    if ( iommu_ipa_bits < p2m_ipa_bits )
>> +        p2m_ipa_bits = iommu_ipa_bits;
>> +}
>> +
>>   /* VTCR value to be configured by all CPUs. Set only once by the 
>> boot CPU */
>>   static uint32_t __read_mostly vtcr;
>>   @@ -1966,10 +1977,28 @@ 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 )
>
> Could we make this IOMMU-agnostic? The main reason to convert from 
> p2m_ipa_bits to pa_range is to cater the rest of the code.
>
> But we could rework the code to do the computation with p2m_ipa_bits 
> and then look-up for the pa_range. 

I am afraid, I don't completely understand your idea of making this 
IOMMU-agnostic and what I should do...


>> +    {
>> +        /*
>> +         * Choose suitable "pa_range" according to the IOMMU 
>> requirements
>> +         * (restricted "p2m_ipa_bits" value).
>> +         * As P2M table is always configured with IPA bits == PA bits,
>> +         * check for equivalent "pabits" and store it's index.
>> +         */
>> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>> +        {
>> +            if ( p2m_ipa_bits == pa_range_info[i].pabits )
>> +            {
>> +                pa_range = i;
>> +                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..413f3e6 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -936,12 +936,19 @@ 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();
>> -
>> +    /*
>> +     * The IOMMU subsystem must be initialized before P2M as we need 
>> to gather
>> +     * requirements regarding the maximum IPA bits supported by each 
>> IOMMU
>> +     * device to figure out the minimum value among them. The P2M 
>> code will
>> +     * choose suitable "pa_range" according to the restricted 
>> "p2m_ipa_bits"
>> +     * value.
>> +     */
>
> This is a bit too verbose, implementation details are not necessary 
> here and increasing the risk to have bit rot comment. So how about:
>
> "The IOMMU subsystem must be initialized before the P2M as we need to 
> gather requirements regarding the maximum IPA bit supported by each 
> IOMMU."?

you are right, will update a comment


>
>
>>       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..d2e36a4 100644
>> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> @@ -1314,23 +1314,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 maximum Stage-2 input size supported by the IPMMU. We 
>> expect
>> +         * the P2M code will take into the account the IOMMU 
>> requirements and
>> +         * choose suitable "pa_range".
>
> I would drop the last sentence.

ok. will do


>
>
>>            */
>> -        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_restrict_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..2b10c6e 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2198,14 +2198,14 @@ 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;
>> -    }
>> -    smmu->s2_input_size = p2m_ipa_bits;
>> +    /*
>> +     * Xen:
>> +     * Set maximum Stage-2 input size supported by the SMMU. We expect
>> +     * the P2M code will take into the account the IOMMU 
>> requirements and
>> +     * choose suitable "pa_range".
>
> Same here.

ok


-- 
Regards,

Oleksandr Tyshchenko


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

Hi,

On 9/10/19 5:24 PM, Oleksandr wrote:
> 
> On 10.09.19 18:11, Julien Grall wrote:
>> Hi Oleksandr,
> 
> Hi, Julien
> 
> 
>>
>> On 8/23/19 8:34 PM, 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 IOMMU's Stage-2 input size must be equal
>>> to the P2M IPA size. It is not a problem when the IOMMU can support
>>> all values the CPU supports. In that case, the IOMMU driver would just
>>> use any "p2m_ipa_bits" value as is. But, there are cases when not.
>>>
>>> In order to make P2M sharing possible on the platforms which
>>> IPMMUs have a limitation in maximum Stage-2 input size introduce
>>> the following logic.
>>>
>>> First initialize the IOMMU subsystem and gather requirements regarding
>>> the maximum IPA bits supported by each IOMMU device to figure out
>>> the minimum value among them. In the P2M code, take into the account
>>> the IOMMU requirements and choose suitable "pa_range" according
>>> to the restricted "p2m_ipa_bits".
>>
>> As I pointed in the previous version, 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.
>> In other words, for Arm32, you need to check p2m_ipa_bits is at least 
>> 40-bits before overriding it.
> 
> But, all modifications with p2m_ipa_bits are done before 
> setup_virt_paging(), where, actually, the p2m_ipa_bits is hard-coded to 
> 40 bits. How can we end up misconfiguring VTCR for ARM32? Or I really 
> missed something?

Sorry if I wasn't cleared, I meant the VTCR for the IOMMU. You would end 
up to configure with a value that is bigger than what it can support.

I am ok if you don't restrict the p2m_ipa_bits and just fail. The point 
is to notify the user ASAP rather than allowing to continue.

This would make the behavior similar to the current implementation 
(although the error would be different).

[...]

>>> +{
>>> +    /*
>>> +     * Calculate the minimum of the maximum IPA bits that any IOMMU
>>> +     * can support.
>>> +     */
>>> +    if ( iommu_ipa_bits < p2m_ipa_bits )
>>> +        p2m_ipa_bits = iommu_ipa_bits;
>>> +}
>>> +
>>>   /* VTCR value to be configured by all CPUs. Set only once by the 
>>> boot CPU */
>>>   static uint32_t __read_mostly vtcr;
>>>   @@ -1966,10 +1977,28 @@ 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 )
>>
>> Could we make this IOMMU-agnostic? The main reason to convert from 
>> p2m_ipa_bits to pa_range is to cater the rest of the code.
>>
>> But we could rework the code to do the computation with p2m_ipa_bits 
>> and then look-up for the pa_range. 
> 
> I am afraid, I don't completely understand your idea of making this 
> IOMMU-agnostic and what I should do...

Roughly what you are doing today is:

if ( iommu_enabled )
    pa_range = find_pa_range_from_p2m_bits().

for_each_cpu()
    if ( cpu.pa_range < pa_range )
      pa_range = cpu.pa_range

....

What you could do is:

for_ech_cpu()
    if ( p2m_ipa_bits < pa_range_info[cpu.pa_range].pabits )
      p2m_ipa_bits = pa_range_info[cpu.pa_range].pabits;

pa_range = find_pa_range_from_p2m_bits();
/* Check validity */
...

Cheers,

-- 
Julien Grall

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

On 10.09.19 21:55, Julien Grall wrote:
> Hi,

Hi Julien


>
> On 9/10/19 5:24 PM, Oleksandr wrote:
>>
>> On 10.09.19 18:11, Julien Grall wrote:
>>> Hi Oleksandr,
>>
>> Hi, Julien
>>
>>
>>>
>>> On 8/23/19 8:34 PM, 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 IOMMU's Stage-2 input size must be 
>>>> equal
>>>> to the P2M IPA size. It is not a problem when the IOMMU can support
>>>> all values the CPU supports. In that case, the IOMMU driver would just
>>>> use any "p2m_ipa_bits" value as is. But, there are cases when not.
>>>>
>>>> In order to make P2M sharing possible on the platforms which
>>>> IPMMUs have a limitation in maximum Stage-2 input size introduce
>>>> the following logic.
>>>>
>>>> First initialize the IOMMU subsystem and gather requirements regarding
>>>> the maximum IPA bits supported by each IOMMU device to figure out
>>>> the minimum value among them. In the P2M code, take into the account
>>>> the IOMMU requirements and choose suitable "pa_range" according
>>>> to the restricted "p2m_ipa_bits".
>>>
>>> As I pointed in the previous version, 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.
>>> In other words, for Arm32, you need to check p2m_ipa_bits is at 
>>> least 40-bits before overriding it.
>>
>> But, all modifications with p2m_ipa_bits are done before 
>> setup_virt_paging(), where, actually, the p2m_ipa_bits is hard-coded 
>> to 40 bits. How can we end up misconfiguring VTCR for ARM32? Or I 
>> really missed something?
>
> Sorry if I wasn't cleared, I meant the VTCR for the IOMMU. You would 
> end up to configure with a value that is bigger than what it can support.
> I am ok if you don't restrict the p2m_ipa_bits and just fail. The 
> point is to notify the user ASAP rather than allowing to continue.
>
> This would make the behavior similar to the current implementation 
> (although the error would be different).

So, in IOMMU driver we should check if IOMMU is able to support at least 
40-bit IPA before trying to restrict. If yes, then go ahead, but if no, 
then just fail. Correct?


>>>> +{
>>>> +    /*
>>>> +     * Calculate the minimum of the maximum IPA bits that any IOMMU
>>>> +     * can support.
>>>> +     */
>>>> +    if ( iommu_ipa_bits < p2m_ipa_bits )
>>>> +        p2m_ipa_bits = iommu_ipa_bits;
>>>> +}
>>>> +
>>>>   /* VTCR value to be configured by all CPUs. Set only once by the 
>>>> boot CPU */
>>>>   static uint32_t __read_mostly vtcr;
>>>>   @@ -1966,10 +1977,28 @@ 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 )
>>>
>>> Could we make this IOMMU-agnostic? The main reason to convert from 
>>> p2m_ipa_bits to pa_range is to cater the rest of the code.
>>>
>>> But we could rework the code to do the computation with p2m_ipa_bits 
>>> and then look-up for the pa_range. 
>>
>> I am afraid, I don't completely understand your idea of making this 
>> IOMMU-agnostic and what I should do...
>
> Roughly what you are doing today is:
>
> if ( iommu_enabled )
>    pa_range = find_pa_range_from_p2m_bits().
>
> for_each_cpu()
>    if ( cpu.pa_range < pa_range )
>      pa_range = cpu.pa_range
>
> ....
>
> What you could do is:

Thank you for the clarification. I think I understand your idea.

But ...

>
> for_ech_cpu()
>    if ( p2m_ipa_bits < pa_range_info[cpu.pa_range].pabits )

Probably you meant ">" here?


> p2m_ipa_bits = pa_range_info[cpu.pa_range].pabits;
>
> pa_range = find_pa_range_from_p2m_bits();
> /* Check validity */


-- 
Regards,

Oleksandr Tyshchenko


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

Hi,

On 9/11/19 5:34 PM, Oleksandr wrote:
> 
> On 10.09.19 21:55, Julien Grall wrote:
>> Hi,
> 
> Hi Julien
> 
> 
>>
>> On 9/10/19 5:24 PM, Oleksandr wrote:
>>>
>>> On 10.09.19 18:11, Julien Grall wrote:
>>>> Hi Oleksandr,
>>>
>>> Hi, Julien
>>>
>>>
>>>>
>>>> On 8/23/19 8:34 PM, 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 IOMMU's Stage-2 input size must be 
>>>>> equal
>>>>> to the P2M IPA size. It is not a problem when the IOMMU can support
>>>>> all values the CPU supports. In that case, the IOMMU driver would just
>>>>> use any "p2m_ipa_bits" value as is. But, there are cases when not.
>>>>>
>>>>> In order to make P2M sharing possible on the platforms which
>>>>> IPMMUs have a limitation in maximum Stage-2 input size introduce
>>>>> the following logic.
>>>>>
>>>>> First initialize the IOMMU subsystem and gather requirements regarding
>>>>> the maximum IPA bits supported by each IOMMU device to figure out
>>>>> the minimum value among them. In the P2M code, take into the account
>>>>> the IOMMU requirements and choose suitable "pa_range" according
>>>>> to the restricted "p2m_ipa_bits".
>>>>
>>>> As I pointed in the previous version, 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.
>>>> In other words, for Arm32, you need to check p2m_ipa_bits is at 
>>>> least 40-bits before overriding it.
>>>
>>> But, all modifications with p2m_ipa_bits are done before 
>>> setup_virt_paging(), where, actually, the p2m_ipa_bits is hard-coded 
>>> to 40 bits. How can we end up misconfiguring VTCR for ARM32? Or I 
>>> really missed something?
>>
>> Sorry if I wasn't cleared, I meant the VTCR for the IOMMU. You would 
>> end up to configure with a value that is bigger than what it can support.
>> I am ok if you don't restrict the p2m_ipa_bits and just fail. The 
>> point is to notify the user ASAP rather than allowing to continue.
>>
>> This would make the behavior similar to the current implementation 
>> (although the error would be different).
> 
> So, in IOMMU driver we should check if IOMMU is able to support at least 
> 40-bit IPA before trying to restrict. If yes, then go ahead, but if no, 
> then just fail. Correct?

There are no need to do this in the IOMU drivers. And I actually don't 
such check in the drivers.

This is the similar problem to half initialized IOMMU. If the user 
requests IOMMU and doesn't work, then we don't want to continue an 
panic. Such check can be done directly in the function setup_virt_paging().

> 
> 
>>>>> +{
>>>>> +    /*
>>>>> +     * Calculate the minimum of the maximum IPA bits that any IOMMU
>>>>> +     * can support.
>>>>> +     */
>>>>> +    if ( iommu_ipa_bits < p2m_ipa_bits )
>>>>> +        p2m_ipa_bits = iommu_ipa_bits;
>>>>> +}
>>>>> +
>>>>>   /* VTCR value to be configured by all CPUs. Set only once by the 
>>>>> boot CPU */
>>>>>   static uint32_t __read_mostly vtcr;
>>>>>   @@ -1966,10 +1977,28 @@ 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 )
>>>>
>>>> Could we make this IOMMU-agnostic? The main reason to convert from 
>>>> p2m_ipa_bits to pa_range is to cater the rest of the code.
>>>>
>>>> But we could rework the code to do the computation with p2m_ipa_bits 
>>>> and then look-up for the pa_range. 
>>>
>>> I am afraid, I don't completely understand your idea of making this 
>>> IOMMU-agnostic and what I should do...
>>
>> Roughly what you are doing today is:
>>
>> if ( iommu_enabled )
>>    pa_range = find_pa_range_from_p2m_bits().
>>
>> for_each_cpu()
>>    if ( cpu.pa_range < pa_range )
>>      pa_range = cpu.pa_range
>>
>> ....
>>
>> What you could do is:
> 
> Thank you for the clarification. I think I understand your idea.
> 
> But ...
> 
>>
>> for_ech_cpu()
>>    if ( p2m_ipa_bits < pa_range_info[cpu.pa_range].pabits )
> 
> Probably you meant ">" here?

Yes.


> 
> 
>> p2m_ipa_bits = pa_range_info[cpu.pa_range].pabits;
>>
>> pa_range = find_pa_range_from_p2m_bits();
>> /* Check validity */
> 
> 

Cheers,

-- 
Julien Grall

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