diff mbox series

[V4] xen/arm: Restrict "p2m_ipa_bits" according to the IOMMU requirements

Message ID 1569340911-20793-1-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series [V4] xen/arm: Restrict "p2m_ipa_bits" according to the IOMMU requirements | expand

Commit Message

Oleksandr Tyshchenko Sept. 24, 2019, 4:01 p.m. UTC 
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
IOMMUs 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>

---
Please note, this patch depends on the following patch series:
https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02282.html
and wasn't checked for the SMMU.

Changes RFC V3 [3] -> V4:
   - Move check for p2m_ipa_bits to be at least 40 bit
     under #ifdef CONFIG_ARM_32
   - Reword the "panic" message

Changes RFC V2 [2] -> RFC V3:
   - Check in setup_virt_paging() that the "restricted"
     P2M IPA size is at least 40-bit
   - Modify logic in setup_virt_paging() a bit to make it
     "IOMMU-agnostic"
   - Clarify comments in code, add some explanations
   - Avoid using the term "IOMMU" in P2M code where possible

Changes RFC V1 [1] -> RFC V2 [2]:
   - 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
[2] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg02237.html
[3] https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg00973.html
---
 xen/arch/arm/p2m.c                       | 41 ++++++++++++++++++++++++++++----
 xen/arch/arm/setup.c                     |  9 +++++--
 xen/drivers/passthrough/arm/ipmmu-vmsa.c | 18 ++------------
 xen/drivers/passthrough/arm/smmu.c       | 11 +++------
 xen/include/asm-arm/p2m.h                |  9 +++++++
 5 files changed, 58 insertions(+), 30 deletions(-)

Comments

Julien Grall Sept. 27, 2019, 10:20 a.m. UTC | #1 
Hi Oleksandr,

Thank you for the respin. The code in p2m.c looks good to me know. One comment regarding the SMMU code below.

On 24/09/2019 17:01, Oleksandr Tyshchenko wrote:
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 8ae986a..701fe9c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2198,14 +2198,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 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. */
> +	p2m_restrict_ipa_bits(size);
> +	smmu->s2_input_size = size;

Sorry I didn't review closely the SMMU code closely until now.

s2_input_size is going to be used by the SMMU to configure the
context banks. However, the number of IPA bits may have been
restricted further by the P2M later on.

So I would squash the following hunk in this patch (untested):

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8ae986a18d..293f428fc7 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1110,7 +1110,11 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
                        reg = TTBCR_TG0_64K;
 
                if (!stage1) {
-                       reg |= (64 - smmu->s2_input_size) << TTBCR_T0SZ_SHIFT;
+                       /*
+                        * Xen: The IOMMU share the page-tables with the P2M
+                        * which may have restrict the size further.
+                        */
+                       reg |= (64 - p2m_ipa_bits) << TTBCR_T0SZ_SHIFT;
 
                        switch (smmu->s2_output_size) {
                        case 32:



>   #if 0
>   	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
>   #ifdef CONFIG_64BIT

Cheers,
Oleksandr Tyshchenko Sept. 27, 2019, 10:26 a.m. UTC | #2 
On 27.09.19 13:20, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> Thank you for the respin. The code in p2m.c looks good to me know. One comment regarding the SMMU code below.
>
> On 24/09/2019 17:01, Oleksandr Tyshchenko wrote:
>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>> index 8ae986a..701fe9c 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2198,14 +2198,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 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. */
>> +	p2m_restrict_ipa_bits(size);
>> +	smmu->s2_input_size = size;
> Sorry I didn't review closely the SMMU code closely until now.
>
> s2_input_size is going to be used by the SMMU to configure the
> context banks. However, the number of IPA bits may have been
> restricted further by the P2M later on.
>
> So I would squash the following hunk in this patch (untested):
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 8ae986a18d..293f428fc7 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1110,7 +1110,11 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
>                          reg = TTBCR_TG0_64K;
>   
>                  if (!stage1) {
> -                       reg |= (64 - smmu->s2_input_size) << TTBCR_T0SZ_SHIFT;
> +                       /*
> +                        * Xen: The IOMMU share the page-tables with the P2M
> +                        * which may have restrict the size further.
> +                        */
> +                       reg |= (64 - p2m_ipa_bits) << TTBCR_T0SZ_SHIFT;
>   
>                          switch (smmu->s2_output_size) {
>                          case 32:


Indeed, makes sense, I will squash.
diff mbox series

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 0f8cc3f..7da41e6 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -34,7 +34,11 @@  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;
+/*
+ * Set larger than any possible value, so the number of IPA 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[] =
@@ -1927,6 +1931,16 @@  struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     return page;
 }
 
+void __init p2m_restrict_ipa_bits(unsigned int ipa_bits)
+{
+    /*
+     * Calculate the minimum of the maximum IPA bits that any external entity
+     * can support.
+     */
+    if ( ipa_bits < p2m_ipa_bits )
+        p2m_ipa_bits = ipa_bits;
+}
+
 /* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
 static uint32_t __read_mostly vtcr;
 
@@ -1958,6 +1972,10 @@  void __init setup_virt_paging(void)
     unsigned long val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
 
 #ifdef CONFIG_ARM_32
+    if ( p2m_ipa_bits < 40 )
+        panic("P2M: Not able to support %u-bit IPA at the moment\n",
+              p2m_ipa_bits);
+
     printk("P2M: 40-bit IPA\n");
     p2m_ipa_bits = 40;
     val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
@@ -1981,15 +1999,20 @@  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;
 
     for_each_online_cpu ( cpu )
     {
         const struct cpuinfo_arm *info = &cpu_data[cpu];
-        if ( info->mm64.pa_range < pa_range )
-            pa_range = info->mm64.pa_range;
+
+        /*
+         * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
+         * with IPA bits == PA bits, compare against "pabits".
+         */
+        if ( pa_range_info[info->mm64.pa_range].pabits < p2m_ipa_bits )
+            p2m_ipa_bits = pa_range_info[info->mm64.pa_range].pabits;
 
         /* Set a flag if the current cpu does not support 16 bit VMIDs. */
         if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
@@ -2003,6 +2026,16 @@  void __init setup_virt_paging(void)
     if ( !vmid_8_bit )
         max_vmid = MAX_VMID_16_BIT;
 
+    /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
+    for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
+    {
+        if ( p2m_ipa_bits == pa_range_info[i].pabits )
+        {
+            pa_range = i;
+            break;
+        }
+    }
+
     /* pa_range is 4 bits, but the defined encodings are only 3 bits */
     if ( pa_range >= ARRAY_SIZE(pa_range_info) || !pa_range_info[pa_range].pabits )
         panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index fca7e68..790eab9 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -928,12 +928,17 @@  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.
+     */
     rc = iommu_setup();
     if ( !iommu_enabled && rc != -ENODEV )
         panic("Couldn't configure correctly all the IOMMUs.\n");
 
+    setup_virt_paging();
+
     do_initcalls();
 
     /*
diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index f2fb4a2..9cfae7e 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -844,22 +844,8 @@  static int ipmmu_probe(struct dt_device_node *node)
             goto out;
         }
 
-        /*
-         * 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.
-         */
-        if ( IPMMU_MAX_P2M_IPA_BITS < p2m_ipa_bits )
-        {
-            printk(XENLOG_ERR "ipmmu: P2M IPA size is not supported (P2M=%u IPMMU=%u)!\n",
-                   p2m_ipa_bits, IPMMU_MAX_P2M_IPA_BITS);
-            ret = -ENODEV;
-            goto out;
-        }
+        /* Set maximum Stage-2 input size supported by the IPMMU. */
+        p2m_restrict_ipa_bits(IPMMU_MAX_P2M_IPA_BITS);
 
         irq = platform_get_irq(node, 0);
         if ( irq < 0 )
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8ae986a..701fe9c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2198,14 +2198,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 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. */
+	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 03f2ee7..89f82df 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -159,6 +159,15 @@  void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
     /* Not supported on ARM. */
 }
 
+/*
+ * Helper to restrict "p2m_ipa_bits" according the external entity
+ * (e.g. IOMMU) requirements.
+ *
+ * Each corresponding driver should report the maximum IPA bits
+ * (Stage-2 input size) it can support.
+ */
+void p2m_restrict_ipa_bits(unsigned int ipa_bits);
+
 /* Second stage paging setup, to be called on all CPUs */
 void setup_virt_paging(void);