[XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[Bhupinder Thakur]

Since VMIDs are related to 2nd stage address translation, it makes more sense
to move the call to p2m_vmid_allocator_init(), which initializes the vmid
allocation bitmap, inside setup_virt_paging(), where 2nd stage address translation
is set up.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 xen/arch/arm/p2m.c        | 3 +++
 xen/arch/arm/setup.c      | 2 --
 xen/include/asm-arm/p2m.h | 3 ---
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index cc5634b..d155c1d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1659,6 +1659,9 @@ void __init setup_virt_paging(void)
 #endif
     printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
            4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
+
+    p2m_vmid_allocator_init();
+
     /* It is not allowed to concatenate a level zero root */
     BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
     setup_virt_paging_one((void *)val);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 38eb888..ac49515 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -789,8 +789,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     gic_init();
 
-    p2m_vmid_allocator_init();
-
     softirq_init();
 
     tasklet_subsys_init();
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index fdb6b47..0987be2 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -152,9 +152,6 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
     /* Not supported on ARM. */
 }
 
-/* Initialise vmid allocator */
-void p2m_vmid_allocator_init(void);
-
 /* 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.xen.org
https://lists.xen.org/xen-devel

Re: [XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[Julien Grall]

Hi Bhupinder,

On 12/12/16 07:26, Bhupinder Thakur wrote:
> Since VMIDs are related to 2nd stage address translation, it makes more sense
> to move the call to p2m_vmid_allocator_init(), which initializes the vmid
> allocation bitmap, inside setup_virt_paging(), where 2nd stage address translation
> is set up.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

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

Regards,

-- 
Julien Grall

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

Re: [XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[Julien Grall]

On 13/12/16 14:06, Julien Grall wrote:
> Hi Bhupinder,
>
> On 12/12/16 07:26, Bhupinder Thakur wrote:
>> Since VMIDs are related to 2nd stage address translation, it makes
>> more sense
>> to move the call to p2m_vmid_allocator_init(), which initializes the vmid
>> allocation bitmap, inside setup_virt_paging(), where 2nd stage address
>> translation
>> is set up.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Actually, one minor change. The function p2m_vmid_allocator_init does 
not need to be exported anymore. So can you please make it static?

With that, you can keep my reviewed-by tag.

Cheers,

-- 
Julien Grall

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

[XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[Bhupinder Thakur]

Since VMIDs are related to 2nd stage address translation, it makes more sense
to move the call to p2m_vmid_allocator_init(), which initializes the vmid
allocation bitmap, inside setup_virt_paging(), where 2nd stage address translation
is set up.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c        | 5 ++++-
 xen/arch/arm/setup.c      | 2 --
 xen/include/asm-arm/p2m.h | 3 ---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index cc5634b..2327509 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1241,7 +1241,7 @@ static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
  */
 static DECLARE_BITMAP(vmid_mask, MAX_VMID);
 
-void p2m_vmid_allocator_init(void)
+static void p2m_vmid_allocator_init(void)
 {
     set_bit(INVALID_VMID, vmid_mask);
 }
@@ -1659,6 +1659,9 @@ void __init setup_virt_paging(void)
 #endif
     printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
            4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
+
+    p2m_vmid_allocator_init();
+
     /* It is not allowed to concatenate a level zero root */
     BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
     setup_virt_paging_one((void *)val);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 38eb888..ac49515 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -789,8 +789,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     gic_init();
 
-    p2m_vmid_allocator_init();
-
     softirq_init();
 
     tasklet_subsys_init();
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index fdb6b47..0987be2 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -152,9 +152,6 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
     /* Not supported on ARM. */
 }
 
-/* Initialise vmid allocator */
-void p2m_vmid_allocator_init(void);
-
 /* 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.xen.org
https://lists.xen.org/xen-devel

[XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Bhupinder Thakur]

VMID space is increased to 16-bits from 8-bits in ARMv8 8.1 revision.
This allows more than 256 VMs to be supported by Xen.

This change adds support for 16-bit VMIDs in Xen based on whether the
architecture supports it.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 xen/arch/arm/p2m.c              | 45 +++++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/p2m.h       |  2 +-
 xen/include/asm-arm/processor.h | 18 ++++++++++++++++-
 3 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2327509..b166122 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -7,6 +7,7 @@
 #include <xen/vm_event.h>
 #include <xen/monitor.h>
 #include <xen/iocap.h>
+#include <xen/xmalloc.h>
 #include <public/vm_event.h>
 #include <asm/flushtlb.h>
 #include <asm/gic.h>
@@ -14,15 +15,23 @@
 #include <asm/hardirq.h>
 #include <asm/page.h>
 
+#define MAX_VMID_8_BIT  (1UL << 8)
+#define MAX_VMID_16_BIT (1UL << 16)
+
+#define INVALID_VMID 0 /* VMID 0 is reserved */
+
 #ifdef CONFIG_ARM_64
 static unsigned int __read_mostly p2m_root_order;
 static unsigned int __read_mostly p2m_root_level;
 #define P2M_ROOT_ORDER    p2m_root_order
 #define P2M_ROOT_LEVEL p2m_root_level
+static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
+#define MAX_VMID       max_vmid
 #else
 /* First level P2M is alway 2 consecutive pages */
 #define P2M_ROOT_LEVEL 1
 #define P2M_ROOT_ORDER    1
+#define MAX_VMID        MAX_VMID_8_BIT
 #endif
 
 #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
@@ -1219,7 +1228,7 @@ static int p2m_alloc_table(struct domain *d)
 
     p2m->root = page;
 
-    p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48;
+    p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid << 48);
 
     /*
      * Make sure that all TLBs corresponding to the new VMID are flushed
@@ -1230,19 +1239,27 @@ static int p2m_alloc_table(struct domain *d)
     return 0;
 }
 
-#define MAX_VMID 256
-#define INVALID_VMID 0 /* VMID 0 is reserved */
 
 static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
 
 /*
- * VTTBR_EL2 VMID field is 8 bits. Using a bitmap here limits us to
- * 256 concurrent domains.
+ * VTTBR_EL2 VMID field is 8 or 16 bits. Aarch64 supports 16-bit VMID. 
+ * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent 
+ * domains. The bitmap space will be allocated dynamically based on 
+ * whether 8 or 16 bit VMIDs are supported.
  */
-static DECLARE_BITMAP(vmid_mask, MAX_VMID);
+static unsigned long *vmid_mask;
 
 static void p2m_vmid_allocator_init(void)
 {
+    /*
+     * allocate space for vmid_mask based on MAX_VMID
+     */
+    vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID));
+
+    if ( !vmid_mask )
+        panic("Could not allocate VMID bitmap space");
+
     set_bit(INVALID_VMID, vmid_mask);
 }
 
@@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void)
 
     unsigned int 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;
+
+        /* set a flag if the current cpu does not suppot 16 bit VMIDs */
+        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
+            vmid_8_bit = true;
     }
 
+    /* 
+     * if the flag is not set then it means all CPUs support 16-bit
+     * VMIDs.
+     */
+    if ( !vmid_8_bit )
+        max_vmid = MAX_VMID_16_BIT;
+
     /* pa_range is 4 bits, but the defined encodings are only 3 bits */
     if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )
         panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
 
     val |= VTCR_PS(pa_range);
     val |= VTCR_TG0_4K;
+
+    /* set the VS bit only if 16 bit VMID is supported */
+    if ( MAX_VMID == MAX_VMID_16_BIT )
+        val |= VTCR_VS;
     val |= VTCR_SL0(pa_range_info[pa_range].sl0);
     val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0987be2..9de55fc 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -30,7 +30,7 @@ struct p2m_domain {
     struct page_info *root;
 
     /* Current VMID in use */
-    uint8_t vmid;
+    uint16_t vmid;
 
     /* Current Translation Table Base Register for the p2m */
     uint64_t vttbr;
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 15bf890..48ce59b 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -215,6 +215,8 @@
 
 #define VTCR_PS(x)      ((x)<<16)
 
+#define VTCR_VS    	    (_AC(0x1,UL)<<19)
+
 #endif
 
 #define VTCR_RES1       (_AC(1,UL)<<31)
@@ -269,6 +271,11 @@
 /* FSR long format */
 #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
 
+#ifdef CONFIG_ARM_64
+#define MM64_VMID_8_BITS_SUPPORT    0x0
+#define MM64_VMID_16_BITS_SUPPORT   0x2
+#endif
+
 #ifndef __ASSEMBLY__
 
 struct cpuinfo_arm {
@@ -337,7 +344,16 @@ struct cpuinfo_arm {
             unsigned long tgranule_64K:4;
             unsigned long tgranule_4K:4;
             unsigned long __res0:32;
-       };
+
+            unsigned long hafdbs:4;
+            unsigned long vmid_bits:4;
+            unsigned long vh:4;
+            unsigned long hpds:4;
+            unsigned long lo:4;
+            unsigned long pan:4;
+            unsigned long __res1:8;
+            unsigned long __res2:32;
+        };
     } mm64;
 
     struct {
-- 
2.7.4


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

Re: [XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[Julien Grall]

Hi Bhupinder,

Nothing related to this patch. I got confused this morning with this 
patch because there is no version numbering, I thought it was just a 
resend, but you did some changes and added my reviewed-by. In the 
future, may I ask you to put the same version number on all the patches 
of the series (in this case v4)?

No need to resend this series if there are no other comments.

Cheers,

On 15/12/16 06:13, Bhupinder Thakur wrote:
> Since VMIDs are related to 2nd stage address translation, it makes more sense
> to move the call to p2m_vmid_allocator_init(), which initializes the vmid
> allocation bitmap, inside setup_virt_paging(), where 2nd stage address translation
> is set up.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/p2m.c        | 5 ++++-
>  xen/arch/arm/setup.c      | 2 --
>  xen/include/asm-arm/p2m.h | 3 ---
>  3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index cc5634b..2327509 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1241,7 +1241,7 @@ static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
>   */
>  static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>
> -void p2m_vmid_allocator_init(void)
> +static void p2m_vmid_allocator_init(void)
>  {
>      set_bit(INVALID_VMID, vmid_mask);
>  }
> @@ -1659,6 +1659,9 @@ void __init setup_virt_paging(void)
>  #endif
>      printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
>             4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
> +
> +    p2m_vmid_allocator_init();
> +
>      /* It is not allowed to concatenate a level zero root */
>      BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
>      setup_virt_paging_one((void *)val);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 38eb888..ac49515 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -789,8 +789,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>
>      gic_init();
>
> -    p2m_vmid_allocator_init();
> -
>      softirq_init();
>
>      tasklet_subsys_init();
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index fdb6b47..0987be2 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -152,9 +152,6 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>      /* Not supported on ARM. */
>  }
>
> -/* Initialise vmid allocator */
> -void p2m_vmid_allocator_init(void);
> -
>  /* Second stage paging setup, to be called on all CPUs */
>  void setup_virt_paging(void);
>
>

-- 
Julien Grall

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

Re: [XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Julien Grall]

Hi Bhupinder,

On 15/12/16 06:13, Bhupinder Thakur wrote:
> VMID space is increased to 16-bits from 8-bits in ARMv8 8.1 revision.
> This allows more than 256 VMs to be supported by Xen.
>
> This change adds support for 16-bit VMIDs in Xen based on whether the
> architecture supports it.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/p2m.c              | 45 +++++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/p2m.h       |  2 +-
>  xen/include/asm-arm/processor.h | 18 ++++++++++++++++-
>  3 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2327509..b166122 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -7,6 +7,7 @@
>  #include <xen/vm_event.h>
>  #include <xen/monitor.h>
>  #include <xen/iocap.h>
> +#include <xen/xmalloc.h>
>  #include <public/vm_event.h>
>  #include <asm/flushtlb.h>
>  #include <asm/gic.h>
> @@ -14,15 +15,23 @@
>  #include <asm/hardirq.h>
>  #include <asm/page.h>
>
> +#define MAX_VMID_8_BIT  (1UL << 8)
> +#define MAX_VMID_16_BIT (1UL << 16)
> +
> +#define INVALID_VMID 0 /* VMID 0 is reserved */
> +
>  #ifdef CONFIG_ARM_64
>  static unsigned int __read_mostly p2m_root_order;
>  static unsigned int __read_mostly p2m_root_level;
>  #define P2M_ROOT_ORDER    p2m_root_order
>  #define P2M_ROOT_LEVEL p2m_root_level
> +static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
> +#define MAX_VMID       max_vmid
>  #else
>  /* First level P2M is alway 2 consecutive pages */
>  #define P2M_ROOT_LEVEL 1
>  #define P2M_ROOT_ORDER    1
> +#define MAX_VMID        MAX_VMID_8_BIT
>  #endif
>
>  #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
> @@ -1219,7 +1228,7 @@ static int p2m_alloc_table(struct domain *d)
>
>      p2m->root = page;
>
> -    p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48;
> +    p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid << 48);
>
>      /*
>       * Make sure that all TLBs corresponding to the new VMID are flushed
> @@ -1230,19 +1239,27 @@ static int p2m_alloc_table(struct domain *d)
>      return 0;
>  }
>
> -#define MAX_VMID 256
> -#define INVALID_VMID 0 /* VMID 0 is reserved */
>
>  static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
>
>  /*
> - * VTTBR_EL2 VMID field is 8 bits. Using a bitmap here limits us to
> - * 256 concurrent domains.
> + * VTTBR_EL2 VMID field is 8 or 16 bits. Aarch64 supports 16-bit VMID.

s/Aarch64/AArch64/

Also I would say "may support" because this is not true for all AArch64 
platform.

> + * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent

s/Aarch64/AArch64/

> + * domains. The bitmap space will be allocated dynamically based on
> + * whether 8 or 16 bit VMIDs are supported.

I am wondering if it would be better to comment #define MAX_VMID 
instead. E.g

/* VMID is by default 8 bit width on AArch64 */
static unsigned int max_vmid = ....;
#define MAX_VMID	max_vmid

/* VMID is always 8 bit width on AArch32 */
#define MAX_VMID	MAX_VMID_8_BIT

What do you think?

>   */
> -static DECLARE_BITMAP(vmid_mask, MAX_VMID);
> +static unsigned long *vmid_mask;
>
>  static void p2m_vmid_allocator_init(void)
>  {
> +    /*
> +     * allocate space for vmid_mask based on MAX_VMID
> +     */
> +    vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID));
> +
> +    if ( !vmid_mask )
> +        panic("Could not allocate VMID bitmap space");
> +
>      set_bit(INVALID_VMID, vmid_mask);
>  }
>
> @@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void)
>
>      unsigned int cpu;
>      unsigned int pa_range = 0x10; /* Larger than any possible value */
> +    bool     vmid_8_bit = false;

Only one space between bool and vmid_8_bit please.

>
>      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;
> +
> +        /* set a flag if the current cpu does not suppot 16 bit VMIDs */

s/set/Set/
s/suppot/support/

Please also add a full stop at the end of the sentence.


> +        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
> +            vmid_8_bit = true;
>      }
>
> +    /*
> +     * if the flag is not set then it means all CPUs support 16-bit

s/if/If/

> +     * VMIDs.
> +     */
> +    if ( !vmid_8_bit )
> +        max_vmid = MAX_VMID_16_BIT;
> +
>      /* pa_range is 4 bits, but the defined encodings are only 3 bits */
>      if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )
>          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);

This code has changed in xen upstream and the platform will unlikely 
apply. Please rebase your code on staging.

>
>      val |= VTCR_PS(pa_range);
>      val |= VTCR_TG0_4K;
> +
> +    /* set the VS bit only if 16 bit VMID is supported */

s/set/Set/ + full stop

> +    if ( MAX_VMID == MAX_VMID_16_BIT )
> +        val |= VTCR_VS;

Sorry, I have only spotted until now. Can you print a message to 
advertise the width of VMID?

See the printk("P2M: %d-bit levels...");

>      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);

Cheers,

-- 
Julien Grall

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

Re: [XEN VMID PATCH 1/2] xen/arm: Move p2m_vmid_allocator_init() inside setup_virt_paging()

[Bhupinder Thakur]

Hi Julien,



On 15 December 2016 at 15:34, Julien Grall <julien.grall@arm.com> wrote:
> Hi Bhupinder,
>
> Nothing related to this patch. I got confused this morning with this patch
> because there is no version numbering, I thought it was just a resend, but
> you did some changes and added my reviewed-by. In the future, may I ask you
> to put the same version number on all the patches of the series (in this
> case v4)?
>
Sorry for the confusion. I will put the same version on all patches in
the series in future.

> No need to resend this series if there are no other comments.
>
> Cheers,
>
>
> On 15/12/16 06:13, Bhupinder Thakur wrote:
>>
>> Since VMIDs are related to 2nd stage address translation, it makes more
>> sense
>> to move the call to p2m_vmid_allocator_init(), which initializes the vmid
>> allocation bitmap, inside setup_virt_paging(), where 2nd stage address
>> translation
>> is set up.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/p2m.c        | 5 ++++-
>>  xen/arch/arm/setup.c      | 2 --
>>  xen/include/asm-arm/p2m.h | 3 ---
>>  3 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index cc5634b..2327509 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1241,7 +1241,7 @@ static spinlock_t vmid_alloc_lock =
>> SPIN_LOCK_UNLOCKED;
>>   */
>>  static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>>
>> -void p2m_vmid_allocator_init(void)
>> +static void p2m_vmid_allocator_init(void)
>>  {
>>      set_bit(INVALID_VMID, vmid_mask);
>>  }
>> @@ -1659,6 +1659,9 @@ void __init setup_virt_paging(void)
>>  #endif
>>      printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
>>             4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
>> +
>> +    p2m_vmid_allocator_init();
>> +
>>      /* It is not allowed to concatenate a level zero root */
>>      BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
>>      setup_virt_paging_one((void *)val);
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 38eb888..ac49515 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -789,8 +789,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>>      gic_init();
>>
>> -    p2m_vmid_allocator_init();
>> -
>>      softirq_init();
>>
>>      tasklet_subsys_init();
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index fdb6b47..0987be2 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -152,9 +152,6 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>>      /* Not supported on ARM. */
>>  }
>>
>> -/* Initialise vmid allocator */
>> -void p2m_vmid_allocator_init(void);
>> -
>>  /* Second stage paging setup, to be called on all CPUs */
>>  void setup_virt_paging(void);
>>
>>
>
> --
> Julien Grall

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

Re: [XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Julien Grall]

Hi Bhupinder,

On 15/12/16 06:13, Bhupinder Thakur wrote:
> +
> +        /* set a flag if the current cpu does not suppot 16 bit VMIDs */
> +        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
> +            vmid_8_bit = true;
>      }
>
> +    /*

I just spot a trailing white space here while trying to apply the patch 
and test it.

> +     * if the flag is not set then it means all CPUs support 16-bit
> +     * VMIDs.
> +     */

Cheers,

-- 
Julien Grall

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

Re: [XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Bhupinder Thakur]

Hi Julien,


>> -#define MAX_VMID 256
>> -#define INVALID_VMID 0 /* VMID 0 is reserved */
>>
>>  static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
>>
>>  /*
>> - * VTTBR_EL2 VMID field is 8 bits. Using a bitmap here limits us to
>> - * 256 concurrent domains.
>> + * VTTBR_EL2 VMID field is 8 or 16 bits. Aarch64 supports 16-bit VMID.
>
>
> s/Aarch64/AArch64/

ok.

>
> Also I would say "may support" because this is not true for all AArch64
> platform.
>
>> + * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent
>
>
> s/Aarch64/AArch64/
>
ok.

>> + * domains. The bitmap space will be allocated dynamically based on
>> + * whether 8 or 16 bit VMIDs are supported.
>
>
> I am wondering if it would be better to comment #define MAX_VMID instead.
> E.g
>
> /* VMID is by default 8 bit width on AArch64 */
> static unsigned int max_vmid = ....;
> #define MAX_VMID        max_vmid
>
> /* VMID is always 8 bit width on AArch32 */
> #define MAX_VMID        MAX_VMID_8_BIT
>
> What do you think?
Looks fine. I will modify.

>
>>   */
>> -static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>> +static unsigned long *vmid_mask;
>>
>>  static void p2m_vmid_allocator_init(void)
>>  {
>> +    /*
>> +     * allocate space for vmid_mask based on MAX_VMID
>> +     */
>> +    vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID));
>> +
>> +    if ( !vmid_mask )
>> +        panic("Could not allocate VMID bitmap space");
>> +
>>      set_bit(INVALID_VMID, vmid_mask);
>>  }
>>
>> @@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void)
>>
>>      unsigned int cpu;
>>      unsigned int pa_range = 0x10; /* Larger than any possible value */
>> +    bool     vmid_8_bit = false;
>
>
> Only one space between bool and vmid_8_bit please.
>
ok.

>>
>>      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;
>> +
>> +        /* set a flag if the current cpu does not suppot 16 bit VMIDs */
>
>
> s/set/Set/
> s/suppot/support/
>
> Please also add a full stop at the end of the sentence.
>
ok.

>
>> +        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
>> +            vmid_8_bit = true;
>>      }
>>
>> +    /*
>> +     * if the flag is not set then it means all CPUs support 16-bit
>
>
> s/if/If/
>
ok.

>> +     * VMIDs.
>> +     */
>> +    if ( !vmid_8_bit )
>> +        max_vmid = MAX_VMID_16_BIT;
>> +
>>      /* pa_range is 4 bits, but the defined encodings are only 3 bits */
>>      if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )
>>          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n",
>> pa_range);
>
>
> This code has changed in xen upstream and the platform will unlikely apply.
> Please rebase your code on staging.
>
I checked with the staging branch. This code is same. I re-based my
code on staging branch but there is no change in this code fragment.

>>
>>      val |= VTCR_PS(pa_range);
>>      val |= VTCR_TG0_4K;
>> +
>> +    /* set the VS bit only if 16 bit VMID is supported */
>
>
> s/set/Set/ + full stop
ok.

>
>> +    if ( MAX_VMID == MAX_VMID_16_BIT )
>> +        val |= VTCR_VS;
>
>
> Sorry, I have only spotted until now. Can you print a message to advertise
> the width of VMID?
>
> See the printk("P2M: %d-bit levels...");
>
ok. I will add the VMID size in this printk.

>>      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>>      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>
>
> Cheers,
>
> --
> Julien Grall

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

Re: [XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Bhupinder Thakur]

Hi Julien,



On 15 December 2016 at 16:00, Julien Grall <julien.grall@arm.com> wrote:
> Hi Bhupinder,
>
> On 15/12/16 06:13, Bhupinder Thakur wrote:
>>
>> +
>> +        /* set a flag if the current cpu does not suppot 16 bit VMIDs */
>> +        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
>> +            vmid_8_bit = true;
>>      }
>>
>> +    /*
>
>
> I just spot a trailing white space here while trying to apply the patch and
> test it.
>
>> +     * if the flag is not set then it means all CPUs support 16-bit
>> +     * VMIDs.
>> +     */
>
I will remove the trailing white space.

>
> Cheers,
>
> --
> Julien Grall

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

Re: [XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Julien Grall]

Hi Bhupinder,

On 15/12/16 11:23, Bhupinder Thakur wrote:.
>
>>> +     * VMIDs.
>>> +     */
>>> +    if ( !vmid_8_bit )
>>> +        max_vmid = MAX_VMID_16_BIT;
>>> +
>>>      /* pa_range is 4 bits, but the defined encodings are only 3 bits */
>>>      if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )
>>>          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n",
>>> pa_range);
>>
>>
>> This code has changed in xen upstream and the platform will unlikely apply.
>> Please rebase your code on staging.
>>
> I checked with the staging branch. This code is same. I re-based my
> code on staging branch but there is no change in this code fragment.

The code was changed by commit 7190f2d "fix potential pa_range_info out 
of bound access" that is present in both staging and master.

I've tried to apply the patch on staging and it failed. So you need to 
rebase it.

Cheers,

-- 
Julien Grall

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