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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel