[Xen-devel] xen/arm: Add support for 16 bit VMIDs

Message ID 1478855206-19185-1-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show

Commit Message

Bhupinder Thakur Nov. 11, 2016, 9:06 a.m.
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              | 44 +++++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/p2m.h       |  2 +-
 xen/include/asm-arm/processor.h | 17 +++++++++++++++-
 3 files changed, 55 insertions(+), 8 deletions(-)

Comments

Stefano Stabellini Nov. 19, 2016, 4:34 a.m. | #1
On Fri, 11 Nov 2016, 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>

Thanks for the patch!


>  xen/arch/arm/p2m.c              | 44 +++++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/p2m.h       |  2 +-
>  xen/include/asm-arm/processor.h | 17 +++++++++++++++-
>  3 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index cc5634b..6ed7e5c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -19,6 +19,7 @@ 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;
>  #else
>  /* First level P2M is alway 2 consecutive pages */
>  #define P2M_ROOT_LEVEL 1
> @@ -1219,7 +1220,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,20 +1231,47 @@ static int p2m_alloc_table(struct domain *d)
>      return 0;
>  }
>  
> -#define MAX_VMID 256
> +#ifdef CONFIG_ARM_64
> +#define MAX_VMID  (1UL << 16)
> +#else
> +#define MAX_VMID (1UL << 8)
> +#endif

Given that MAX_VMID on ARM64 can be either 256 or 65536, and given that
this patch also introduces max_vmid, I find these #defines confusing. It
is not obvious how max_vmid and MAX_VMID differ. I would go for
something like the following:

#define MAX_VMID_8  (1UL << 8) 
#define MAX_VMID_16 (1UL << 16) 
#ifdef CONFIG_ARM_64
#define MAX_VMID_ARCH MAX_VMID_16
#else
#define MAX_VMID_ARCH MAX_VMID_8
#endif

>  #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.
>   */
>  static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>  
>  void p2m_vmid_allocator_init(void)
>  {
> +    unsigned int cpu;
> +
>      set_bit(INVALID_VMID, vmid_mask);
> +
> +    max_vmid = MAX_VMID;
> +
> +#ifdef CONFIG_ARM_64
> +    /* 
> +     * if any cpu does not support 16-bit VMID then restrict the 
> +     * max VMIDs which can be allocated to 256
> +     */
> +    for_each_online_cpu ( cpu )
> +    {
> +        const struct cpuinfo_arm *info = &cpu_data[cpu];
> +
> +        if ( info->mm64.vmid_bits != VMID_16_BITS_SUPPORT )
> +        {
> +            max_vmid = (1UL << 8);
> +            break;
> +        }
> +    }
> +#endif
>  }
>  
>  static int p2m_alloc_vmid(struct domain *d)
> @@ -1254,11 +1282,11 @@ static int p2m_alloc_vmid(struct domain *d)
>  
>      spin_lock(&vmid_alloc_lock);
>  
> -    nr = find_first_zero_bit(vmid_mask, MAX_VMID);
> +    nr = find_first_zero_bit(vmid_mask, max_vmid);
>  
>      ASSERT(nr != INVALID_VMID);
>  
> -    if ( nr == MAX_VMID )
> +    if ( nr == max_vmid )
>      {
>          rc = -EBUSY;
>          printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id);
> @@ -1646,6 +1674,10 @@ void __init setup_virt_paging(void)
>  
>      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 )
> +        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 fdb6b47..bfcdbf1 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..4b6be3d 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -215,6 +215,7 @@
>  
>  #define VTCR_PS(x)      ((x)<<16)
>  
> +#define VTCR_VS    	(_AC(0x1,UL)<<19)
>  #endif
>  
>  #define VTCR_RES1       (_AC(1,UL)<<31)
> @@ -269,6 +270,11 @@
>  /* FSR long format */
>  #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
>  
> +#ifdef CONFIG_ARM_64
> +#define VMID_8_BITS_SUPPORT         0x0
> +#define VMID_16_BITS_SUPPORT        0x2
> +#endif
> +
>  #ifndef __ASSEMBLY__
>  
>  struct cpuinfo_arm {
> @@ -337,7 +343,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
>
Julien Grall Nov. 22, 2016, 4:07 p.m. | #2
Hi Stefano,

On 19/11/16 04:34, Stefano Stabellini wrote:
> On Fri, 11 Nov 2016, 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              | 44 +++++++++++++++++++++++++++++++++++------
>>  xen/include/asm-arm/p2m.h       |  2 +-
>>  xen/include/asm-arm/processor.h | 17 +++++++++++++++-
>>  3 files changed, 55 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index cc5634b..6ed7e5c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -19,6 +19,7 @@ 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;
>>  #else
>>  /* First level P2M is alway 2 consecutive pages */
>>  #define P2M_ROOT_LEVEL 1
>> @@ -1219,7 +1220,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,20 +1231,47 @@ static int p2m_alloc_table(struct domain *d)
>>      return 0;
>>  }
>>
>> -#define MAX_VMID 256
>> +#ifdef CONFIG_ARM_64
>> +#define MAX_VMID  (1UL << 16)
>> +#else
>> +#define MAX_VMID (1UL << 8)
>> +#endif
>
> Given that MAX_VMID on ARM64 can be either 256 or 65536, and given that
> this patch also introduces max_vmid, I find these #defines confusing. It
> is not obvious how max_vmid and MAX_VMID differ. I would go for
> something like the following:
>
> #define MAX_VMID_8  (1UL << 8)
> #define MAX_VMID_16 (1UL << 16)
> #ifdef CONFIG_ARM_64
> #define MAX_VMID_ARCH MAX_VMID_16
> #else
> #define MAX_VMID_ARCH MAX_VMID_8
> #endif

MAX_VMID is mostly used to define the vmid at compilation time.
However, with the support of 16 bits VMID, the size is now 8KB. So we 
would increase Xen footprint by 8KB on all AArch64 platform (even non 
ARMv8.1 compliant).

I would much prefer to see this bitmap allocating at runtime. With that 
we could drop the use of MAX_VMID.

Regards,
Julien Grall Nov. 22, 2016, 5:47 p.m. | #3
Hello Bhupinder,

On 11/11/16 09:06, 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              | 44 +++++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/p2m.h       |  2 +-
>  xen/include/asm-arm/processor.h | 17 +++++++++++++++-
>  3 files changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index cc5634b..6ed7e5c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -19,6 +19,7 @@ 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;
>  #else
>  /* First level P2M is alway 2 consecutive pages */
>  #define P2M_ROOT_LEVEL 1
> @@ -1219,7 +1220,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,20 +1231,47 @@ static int p2m_alloc_table(struct domain *d)
>      return 0;
>  }
>
> -#define MAX_VMID 256
> +#ifdef CONFIG_ARM_64
> +#define MAX_VMID  (1UL << 16)
> +#else
> +#define MAX_VMID (1UL << 8)
> +#endif
> +
>  #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.
>   */
>  static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>
>  void p2m_vmid_allocator_init(void)
>  {
> +    unsigned int cpu;
> +
>      set_bit(INVALID_VMID, vmid_mask);
> +
> +    max_vmid = MAX_VMID;

max_vmid is only declared for ARM64 and will break compilation for 
ARM32. Please try to build each patch for both architecture before 
sending to the ML.

However, in this case. It would make more sense to keep the maximum vmid 
static for ARM32 as it will never change.

I quite like what has been done for P2M_ROOT_{LEVEL,ORDER} where the 
define is a constant on ARM32 and point to a variable on ARM64.

> +
> +#ifdef CONFIG_ARM_64
> +    /*
> +     * if any cpu does not support 16-bit VMID then restrict the
> +     * max VMIDs which can be allocated to 256
> +     */
> +    for_each_online_cpu ( cpu )
> +    {
> +        const struct cpuinfo_arm *info = &cpu_data[cpu];
> +
> +        if ( info->mm64.vmid_bits != VMID_16_BITS_SUPPORT )
> +        {
> +            max_vmid = (1UL << 8);
> +            break;
> +        }
> +    }
> +#endif

I would rework this code to have max_vmid set to 8 bits by default and 
the turn on 16 bits if available on every CPU.

>  }
>
>  static int p2m_alloc_vmid(struct domain *d)
> @@ -1254,11 +1282,11 @@ static int p2m_alloc_vmid(struct domain *d)
>
>      spin_lock(&vmid_alloc_lock);
>
> -    nr = find_first_zero_bit(vmid_mask, MAX_VMID);
> +    nr = find_first_zero_bit(vmid_mask, max_vmid);
>
>      ASSERT(nr != INVALID_VMID);
>
> -    if ( nr == MAX_VMID )
> +    if ( nr == max_vmid )
>      {
>          rc = -EBUSY;
>          printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id);
> @@ -1646,6 +1674,10 @@ void __init setup_virt_paging(void)
>
>      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 )

This check is confusing, I would be clearer to use MAX_VMID_16 or else.

> +        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 fdb6b47..bfcdbf1 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..4b6be3d 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -215,6 +215,7 @@
>
>  #define VTCR_PS(x)      ((x)<<16)
>
> +#define VTCR_VS    	(_AC(0x1,UL)<<19)

Newline here please.

>  #endif
>
>  #define VTCR_RES1       (_AC(1,UL)<<31)
> @@ -269,6 +270,11 @@
>  /* FSR long format */
>  #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
>
> +#ifdef CONFIG_ARM_64
> +#define VMID_8_BITS_SUPPORT         0x0
> +#define VMID_16_BITS_SUPPORT        0x2
> +#endif

I would prefix the 2 define with MM64_ so we know how the values will be 
matched.

> +
>  #ifndef __ASSEMBLY__
>
>  struct cpuinfo_arm {
> @@ -337,7 +343,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 {
>

Regards,
Stefano Stabellini Nov. 22, 2016, 6:01 p.m. | #4
On Tue, 22 Nov 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 19/11/16 04:34, Stefano Stabellini wrote:
> > On Fri, 11 Nov 2016, 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              | 44
> > > +++++++++++++++++++++++++++++++++++------
> > >  xen/include/asm-arm/p2m.h       |  2 +-
> > >  xen/include/asm-arm/processor.h | 17 +++++++++++++++-
> > >  3 files changed, 55 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index cc5634b..6ed7e5c 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -19,6 +19,7 @@ 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;
> > >  #else
> > >  /* First level P2M is alway 2 consecutive pages */
> > >  #define P2M_ROOT_LEVEL 1
> > > @@ -1219,7 +1220,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,20 +1231,47 @@ static int p2m_alloc_table(struct domain *d)
> > >      return 0;
> > >  }
> > > 
> > > -#define MAX_VMID 256
> > > +#ifdef CONFIG_ARM_64
> > > +#define MAX_VMID  (1UL << 16)
> > > +#else
> > > +#define MAX_VMID (1UL << 8)
> > > +#endif
> > 
> > Given that MAX_VMID on ARM64 can be either 256 or 65536, and given that
> > this patch also introduces max_vmid, I find these #defines confusing. It
> > is not obvious how max_vmid and MAX_VMID differ. I would go for
> > something like the following:
> > 
> > #define MAX_VMID_8  (1UL << 8)
> > #define MAX_VMID_16 (1UL << 16)
> > #ifdef CONFIG_ARM_64
> > #define MAX_VMID_ARCH MAX_VMID_16
> > #else
> > #define MAX_VMID_ARCH MAX_VMID_8
> > #endif
> 
> MAX_VMID is mostly used to define the vmid at compilation time.
> However, with the support of 16 bits VMID, the size is now 8KB. So we would
> increase Xen footprint by 8KB on all AArch64 platform (even non ARMv8.1
> compliant).
> 
> I would much prefer to see this bitmap allocating at runtime. With that we
> could drop the use of MAX_VMID.

You are right
Bhupinder Thakur Nov. 23, 2016, 10:38 a.m. | #5
Hi,


 static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>>

>>  void p2m_vmid_allocator_init(void)

>>  {

>> +    unsigned int cpu;

>> +

>>      set_bit(INVALID_VMID, vmid_mask);

>> +

>> +    max_vmid = MAX_VMID;

>>

>

> max_vmid is only declared for ARM64 and will break compilation for ARM32.

> Please try to build each patch for both architecture before sending to the

> ML.

>

> I will compile for ARM32.



> However, in this case. It would make more sense to keep the maximum vmid

> static for ARM32 as it will never change.

>

>

I quite like what has been done for P2M_ROOT_{LEVEL,ORDER} where the define
> is a constant on ARM32 and point to a variable on ARM64.

>

> I will keep the bitmap statically defined for ARM32. For ARM64 bitmap will

be allocated dynamically.


> +

>> +#ifdef CONFIG_ARM_64

>> +    /*

>> +     * if any cpu does not support 16-bit VMID then restrict the

>> +     * max VMIDs which can be allocated to 256

>> +     */

>> +    for_each_online_cpu ( cpu )

>> +    {

>> +        const struct cpuinfo_arm *info = &cpu_data[cpu];

>> +

>> +        if ( info->mm64.vmid_bits != VMID_16_BITS_SUPPORT )

>> +        {

>> +            max_vmid = (1UL << 8);

>> +            break;

>> +        }

>> +    }

>> +#endif

>>

>

> I would rework this code to have max_vmid set to 8 bits by default and the

> turn on 16 bits if available on every CPU.

>

> I will modify the code accordingly.



>  }

>>

>>  static int p2m_alloc_vmid(struct domain *d)

>> @@ -1254,11 +1282,11 @@ static int p2m_alloc_vmid(struct domain *d)

>>

>>      spin_lock(&vmid_alloc_lock);

>>

>> -    nr = find_first_zero_bit(vmid_mask, MAX_VMID);

>> +    nr = find_first_zero_bit(vmid_mask, max_vmid);

>>

>>      ASSERT(nr != INVALID_VMID);

>>

>> -    if ( nr == MAX_VMID )

>> +    if ( nr == max_vmid )

>>      {

>>          rc = -EBUSY;

>>          printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n",

>> d->domain_id);

>> @@ -1646,6 +1674,10 @@ void __init setup_virt_paging(void)

>>

>>      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 )

>>

>

> This check is confusing, I would be clearer to use MAX_VMID_16 or else.

>

> I will explicitly use 16 bit macro



> +        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 fdb6b47..bfcdbf1 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..4b6be3d 100644

>> --- a/xen/include/asm-arm/processor.h

>> +++ b/xen/include/asm-arm/processor.h

>> @@ -215,6 +215,7 @@

>>

>>  #define VTCR_PS(x)      ((x)<<16)

>>

>> +#define VTCR_VS        (_AC(0x1,UL)<<19)

>>

>

> Newline here please.

>

> Ok.


>  #endif

>>

>>  #define VTCR_RES1       (_AC(1,UL)<<31)

>> @@ -269,6 +270,11 @@

>>  /* FSR long format */

>>  #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)

>>

>> +#ifdef CONFIG_ARM_64

>> +#define VMID_8_BITS_SUPPORT         0x0

>> +#define VMID_16_BITS_SUPPORT        0x2

>> +#endif

>>

>

> I would prefix the 2 define with MM64_ so we know how the values will be

> matched.

>

> You mean I define them like this ?

#define MM64_VMID_8_BITS_SUPPORT   0x0
#define MM64_VMID_16_BITS_SUPPORT 0x2

+
>>  #ifndef __ASSEMBLY__

>>

>>  struct cpuinfo_arm {

>> @@ -337,7 +343,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 {

>>

>>

> Regards,

>

> --

> Julien Grall

>
Julien Grall Nov. 23, 2016, 1:12 p.m. | #6
On 23/11/16 10:38, Bhupinder Thakur wrote:
> Hi,

Hi Bhupinder,

Can you configure your e-mail client to use ">" to quote the previous 
answer? Using tab makes the mail quite confusing. Thank you.

>
>          static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>
>          void p2m_vmid_allocator_init(void)
>          {
>         +    unsigned int cpu;
>         +
>              set_bit(INVALID_VMID, vmid_mask);
>         +
>         +    max_vmid = MAX_VMID;
>
>
>     max_vmid is only declared for ARM64 and will break compilation for
>     ARM32. Please try to build each patch for both architecture before
>     sending to the ML.
>
> I will compile for ARM32.
>
>
>     However, in this case. It would make more sense to keep the maximum
>     vmid static for ARM32 as it will never change.
>
>
>     I quite like what has been done for P2M_ROOT_{LEVEL,ORDER} where the
>     define is a constant on ARM32 and point to a variable on ARM64.
>
> I will keep the bitmap statically defined for ARM32. For ARM64 bitmap
> will be allocated dynamically.

I am not asking the bitmap to be static but the number of vmid to be a 
constant (not a variable) for ARM32.

I prefer to have the bitmap dynamic on both architecture because it is 
will be easier to maintain the code in long term.

What I was asking for is:

#ifdef CONFIG_ARM_64
/* The VMID is encoded on 8-bit minimum. */
static unsigned int __read_mostly max_vmid = MAX_VMID_8BIT;
#define MAX_VMID max_vmid
#else
/* The VMID is always encoded on 8-bit */
#define MAX_VMID MAX_VMID_8BIT
#endif

The code would use MAX_VMID and the variable max_vmid would only be 
defined for arm64 breaking the compilation directly if used in the 
common code.

[...]

>          #endif
>
>          #define VTCR_RES1       (_AC(1,UL)<<31)
>         @@ -269,6 +270,11 @@
>          /* FSR long format */
>          #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
>
>         +#ifdef CONFIG_ARM_64
>         +#define VMID_8_BITS_SUPPORT         0x0
>         +#define VMID_16_BITS_SUPPORT        0x2
>         +#endif
>
>
>     I would prefix the 2 define with MM64_ so we know how the values
>     will be matched.
>
> You mean I define them like this ?
> #define MM64_VMID_8_BITS_SUPPORT   0x0
> #define MM64_VMID_16_BITS_SUPPORT 0x2

Yes, or any other name mentioning the register name in the define. This 
is how are defined all the register constant within this file.

Regards,

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index cc5634b..6ed7e5c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -19,6 +19,7 @@  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;
 #else
 /* First level P2M is alway 2 consecutive pages */
 #define P2M_ROOT_LEVEL 1
@@ -1219,7 +1220,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,20 +1231,47 @@  static int p2m_alloc_table(struct domain *d)
     return 0;
 }
 
-#define MAX_VMID 256
+#ifdef CONFIG_ARM_64
+#define MAX_VMID  (1UL << 16)
+#else
+#define MAX_VMID (1UL << 8)
+#endif
+
 #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.
  */
 static DECLARE_BITMAP(vmid_mask, MAX_VMID);
 
 void p2m_vmid_allocator_init(void)
 {
+    unsigned int cpu;
+
     set_bit(INVALID_VMID, vmid_mask);
+
+    max_vmid = MAX_VMID;
+
+#ifdef CONFIG_ARM_64
+    /* 
+     * if any cpu does not support 16-bit VMID then restrict the 
+     * max VMIDs which can be allocated to 256
+     */
+    for_each_online_cpu ( cpu )
+    {
+        const struct cpuinfo_arm *info = &cpu_data[cpu];
+
+        if ( info->mm64.vmid_bits != VMID_16_BITS_SUPPORT )
+        {
+            max_vmid = (1UL << 8);
+            break;
+        }
+    }
+#endif
 }
 
 static int p2m_alloc_vmid(struct domain *d)
@@ -1254,11 +1282,11 @@  static int p2m_alloc_vmid(struct domain *d)
 
     spin_lock(&vmid_alloc_lock);
 
-    nr = find_first_zero_bit(vmid_mask, MAX_VMID);
+    nr = find_first_zero_bit(vmid_mask, max_vmid);
 
     ASSERT(nr != INVALID_VMID);
 
-    if ( nr == MAX_VMID )
+    if ( nr == max_vmid )
     {
         rc = -EBUSY;
         printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id);
@@ -1646,6 +1674,10 @@  void __init setup_virt_paging(void)
 
     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 )
+        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 fdb6b47..bfcdbf1 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..4b6be3d 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -215,6 +215,7 @@ 
 
 #define VTCR_PS(x)      ((x)<<16)
 
+#define VTCR_VS    	(_AC(0x1,UL)<<19)
 #endif
 
 #define VTCR_RES1       (_AC(1,UL)<<31)
@@ -269,6 +270,11 @@ 
 /* FSR long format */
 #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
 
+#ifdef CONFIG_ARM_64
+#define VMID_8_BITS_SUPPORT         0x0
+#define VMID_16_BITS_SUPPORT        0x2
+#endif
+
 #ifndef __ASSEMBLY__
 
 struct cpuinfo_arm {
@@ -337,7 +343,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 {