Re: [Xen-devel] [PATCH] xen: arm: improve VMID allocation.

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 09/06/2013 12:24 PM, Ian Campbell wrote:
> The VMID field is 8 bits. Rather than allowing only up to 256 VMs per host
> reboot before things start "acting strange" instead maintain a simple bitmap
> of used VMIDs and allocate them statically to guests upon creation.
> 
> This limits us to 256 concurrent VMs which is a reasonable improvement.
> Eventually we will want a proper scheme to allocate VMIDs on context switch.
> 
> The existing code reserves VMID==0, IIRC I thought this was "hypervisor
> context". Re-reading the ARM ARM I'm not spotting the bit which made me think
> this. However I haven't changed this behaviour here.

The ARM ARM only says the VMID==0 is used during reset, so I think we
can assume that ID is reserved.

> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> 
> NB: compile test only.
> ---
>  xen/arch/arm/p2m.c        |   57 
> +++++++++++++++++++++++++++++++++++++++++----
>  xen/arch/arm/setup.c      |    2 ++
>  xen/include/asm-arm/p2m.h |    3 +++
>  3 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 307c6d4..e3dfbd7 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -3,6 +3,7 @@
>  #include <xen/lib.h>
>  #include <xen/errno.h>
>  #include <xen/domain_page.h>
> +#include <xen/bitops.h>
>  #include <asm/flushtlb.h>
>  #include <asm/gic.h>
>  
> @@ -306,6 +307,46 @@ int p2m_alloc_table(struct domain *d)
>      return 0;
>  }
>  
> +#define MAX_VMID 256
> +
> +/* VTTBR_EL2 VMID field is 8 bits. Using a bitmap here limits us to
> + * 256 concurrent domains. */
> +DECLARE_BITMAP(vmid_mask, MAX_VMID);

static DECLARE_...

> +
> +void p2m_vmid_allocator_init(void)
> +{
> +    /* VMID 0 is reserved */
> +    set_bit(0, vmid_mask);
> +}
> +
> +/* p2m_alloc|free_vmid must both be called with the p2m->lock */

Taking p2m->lock is not enough, this function can be called concurrently
if the 2 domains are created at the same time.
In this case, it's possible to have the same vmid for 2 domains.
How about a global lock?

> +static int p2m_alloc_vmid(struct domain *d)
> +{
> +    struct p2m_domain *p2m = &d->arch.p2m;
> +
> +    int nr = find_first_zero_bit(vmid_mask, MAX_VMID);
> +
> +    ASSERT(nr > 0); /* VMID is reserved */
> +
> +    if ( nr == MAX_VMID )
> +    {
> +        printk("p2m.c: dom%d: VMID pool exhausted\n", d->domain_id);
printk(XENLOG_ERR...?

> +        return -EBUSY;
> +    }
> +
> +    set_bit(nr, vmid_mask);
> +
> +    p2m->vmid = nr;
> +
> +    return 0;
> +}
> +
> +static void p2m_free_vmid(struct domain *d)
> +{
> +    struct p2m_domain *p2m = &d->arch.p2m;
> +    clear_bit(p2m->vmid, vmid_mask);
> +}
> +
>  void p2m_teardown(struct domain *d)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> @@ -318,25 +359,33 @@ void p2m_teardown(struct domain *d)
>  
>      p2m->first_level = NULL;
>  
> +    p2m_free_vmid(d);
> +

p2m_teardown is called when the domain is destroyed. If the vmid was not
correctly set (for instance because VMID pool exhausted), we will clear
the wrong bit (here 0).
At the next domain creation, we will assert because nr = 0.

>      spin_unlock(&p2m->lock);
>  }
>  
>  int p2m_init(struct domain *d)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> +    int rc = 0;
>  
>      spin_lock_init(&p2m->lock);
>      INIT_PAGE_LIST_HEAD(&p2m->pages);
>  
> -    /* XXX allocate properly */
> -    /* Zero is reserved */
> -    p2m->vmid = d->domain_id + 1;
> +    spin_lock(&p2m->lock);
> +
> +    rc = p2m_alloc_vmid(d);
> +    if ( rc != 0 )
> +        goto err;
>  
>      d->arch.vttbr = 0;
>  
>      p2m->first_level = NULL;
>  
> -    return 0;
> +err:
> +    spin_unlock(&p2m->lock);
> +
> +    return rc;
>  }
>  
>  unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4b31623..4dfb56f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -544,6 +544,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      setup_virt_paging();
>  
> +    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 a00069b..c660820 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -20,6 +20,9 @@ struct p2m_domain {
>      uint8_t vmid;
>  };
>  
> +/* Initialise vmid allocator */
> +void p2m_vmid_allocator_init(void);
> +
>  /* Init the datastructures for later use by the p2m code */
>  int p2m_init(struct domain *d);
>  
> 


-- 
Julien Grall

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