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

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.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---

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);
+
+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 */
+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);
+        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);
+
     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);
 
-- 
1.7.10.4

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@citrix.com>
> ---
> 
> 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

On Wed, 2013-09-11 at 13:44 +0100, Julien Grall wrote:
> > +
> > +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?
Brain fart, I forgot this lock was per p2m.

The domain lock is probably OK to use here, in fact we may already hold
it. I''ll fix this.
> > @@ -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.
Oops! Will fix.

Ian.