[PATCH 0/3] xen: Improvements to the vcpu create/destroy paths

[PATCH 0/3] xen: Improvements to the vcpu create/destroy paths

[Andrew Cooper]

This is the start of the work to make the vcpu_destroy() code idempotent, so
vcpu construction can be moved into domain_create() and successfully unwound
on error.

Andrew Cooper (3):
  xen/vcpu: Rename the common interfaces for consistency
  xen/vcpu: Introduce vcpu_destroy()
  xen/vcpu: Rework sanity checks in vcpu_create()

 xen/arch/arm/domain.c       |  6 ++--
 xen/arch/arm/domain_build.c |  4 +--
 xen/arch/arm/setup.c        |  1 -
 xen/arch/x86/dom0_build.c   |  2 +-
 xen/arch/x86/domain.c       |  4 +--
 xen/arch/x86/setup.c        |  1 -
 xen/common/domain.c         | 80 ++++++++++++++++++++++++++++++---------------
 xen/common/domctl.c         |  2 +-
 xen/common/schedule.c       |  4 +--
 xen/include/xen/domain.h    | 10 +++---
 10 files changed, 69 insertions(+), 45 deletions(-)

-- 
2.1.4


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

[PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency

[Andrew Cooper]

The vcpu functions are far less consistent than the domain side of things, and
in particular, has vcpu_destroy() for architecture specific functionality.

Perform the following renames:

  * alloc_vcpu      => vcpu_create
  * vcpu_initialise => arch_vcpu_create
  * vcpu_destroy    => arch_vcpu_destroy

which makes the vcpu hierarchy consistent with the domain hierarchy.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain.c       |  6 +++---
 xen/arch/arm/domain_build.c |  4 ++--
 xen/arch/x86/dom0_build.c   |  2 +-
 xen/arch/x86/domain.c       |  4 ++--
 xen/common/domain.c         |  6 +++---
 xen/common/domctl.c         |  2 +-
 xen/common/schedule.c       |  4 ++--
 xen/include/xen/domain.h    | 10 +++++-----
 8 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4baecc2..feebbf5 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -538,7 +538,7 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_pages(v, get_order_from_bytes(sizeof(*v)));
 }
 
-int vcpu_initialise(struct vcpu *v)
+int arch_vcpu_create(struct vcpu *v)
 {
     int rc = 0;
 
@@ -583,11 +583,11 @@ int vcpu_initialise(struct vcpu *v)
     return rc;
 
 fail:
-    vcpu_destroy(v);
+    arch_vcpu_destroy(v);
     return rc;
 }
 
-void vcpu_destroy(struct vcpu *v)
+void arch_vcpu_destroy(struct vcpu *v)
 {
     vcpu_timer_destroy(v);
     vcpu_vgic_free(v);
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2a383c8..5057ad8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -74,7 +74,7 @@ unsigned int __init dom0_max_vcpus(void)
 
 struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
 {
-    return alloc_vcpu(dom0, 0, 0);
+    return vcpu_create(dom0, 0, 0);
 }
 
 static unsigned int __init get_11_allocation_size(paddr_t size)
@@ -2232,7 +2232,7 @@ int __init construct_dom0(struct domain *d)
     for ( i = 1, cpu = 0; i < d->max_vcpus; i++ )
     {
         cpu = cpumask_cycle(cpu, &cpu_online_map);
-        if ( alloc_vcpu(d, i, cpu) == NULL )
+        if ( vcpu_create(d, i, cpu) == NULL )
         {
             printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu);
             break;
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 423fdec..86eb7db 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -134,7 +134,7 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d,
                                     unsigned int prev_cpu)
 {
     unsigned int cpu = cpumask_cycle(prev_cpu, &dom0_cpus);
-    struct vcpu *v = alloc_vcpu(d, vcpu_id, cpu);
+    struct vcpu *v = vcpu_create(d, vcpu_id, cpu);
 
     if ( v )
     {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 313ebb3..d67a047 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -322,7 +322,7 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_page(v);
 }
 
-int vcpu_initialise(struct vcpu *v)
+int arch_vcpu_create(struct vcpu *v)
 {
     struct domain *d = v->domain;
     int rc;
@@ -382,7 +382,7 @@ int vcpu_initialise(struct vcpu *v)
     return rc;
 }
 
-void vcpu_destroy(struct vcpu *v)
+void arch_vcpu_destroy(struct vcpu *v)
 {
     xfree(v->arch.vm_event);
     v->arch.vm_event = NULL;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 78c450e..14c8d00 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -123,7 +123,7 @@ static void vcpu_info_reset(struct vcpu *v)
     v->vcpu_info_mfn = INVALID_MFN;
 }
 
-struct vcpu *alloc_vcpu(
+struct vcpu *vcpu_create(
     struct domain *d, unsigned int vcpu_id, unsigned int cpu_id)
 {
     struct vcpu *v;
@@ -165,7 +165,7 @@ struct vcpu *alloc_vcpu(
     if ( sched_init_vcpu(v, cpu_id) != 0 )
         goto fail_wq;
 
-    if ( vcpu_initialise(v) != 0 )
+    if ( arch_vcpu_create(v) != 0 )
     {
         sched_destroy_vcpu(v);
  fail_wq:
@@ -870,7 +870,7 @@ static void complete_domain_destroy(struct rcu_head *head)
         if ( (v = d->vcpu[i]) == NULL )
             continue;
         tasklet_kill(&v->continue_hypercall_tasklet);
-        vcpu_destroy(v);
+        arch_vcpu_destroy(v);
         sched_destroy_vcpu(v);
         destroy_waitqueue_vcpu(v);
     }
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ed047b7..2facbdb 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -571,7 +571,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                 cpumask_any(online) :
                 cpumask_cycle(d->vcpu[i-1]->processor, online);
 
-            if ( alloc_vcpu(d, i, cpu) == NULL )
+            if ( vcpu_create(d, i, cpu) == NULL )
                 goto maxvcpu_out;
         }
 
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 05281d6..1ef5be9 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1645,7 +1645,7 @@ static int cpu_schedule_up(unsigned int cpu)
         return 0;
 
     if ( idle_vcpu[cpu] == NULL )
-        alloc_vcpu(idle_vcpu[0]->domain, cpu, cpu);
+        vcpu_create(idle_vcpu[0]->domain, cpu, cpu);
     else
     {
         struct vcpu *idle = idle_vcpu[cpu];
@@ -1817,7 +1817,7 @@ void __init scheduler_init(void)
     BUG_ON(IS_ERR(idle_domain));
     idle_domain->vcpu = idle_vcpu;
     idle_domain->max_vcpus = nr_cpu_ids;
-    if ( alloc_vcpu(idle_domain, 0, 0) == NULL )
+    if ( vcpu_create(idle_domain, 0, 0) == NULL )
         BUG();
     this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0);
     BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv));
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 5593495..6df6a58 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -13,7 +13,7 @@ typedef union {
     struct compat_vcpu_guest_context *cmp;
 } vcpu_guest_context_u __attribute__((__transparent_union__));
 
-struct vcpu *alloc_vcpu(
+struct vcpu *vcpu_create(
     struct domain *d, unsigned int vcpu_id, unsigned int cpu_id);
 
 unsigned int dom0_max_vcpus(void);
@@ -47,13 +47,13 @@ void free_pirq_struct(void *);
 
 /*
  * Initialise/destroy arch-specific details of a VCPU.
- *  - vcpu_initialise() is called after the basic generic fields of the
+ *  - arch_vcpu_create() is called after the basic generic fields of the
  *    VCPU structure are initialised. Many operations can be applied to the
  *    VCPU at this point (e.g., vcpu_pause()).
- *  - vcpu_destroy() is called only if vcpu_initialise() previously succeeded.
+ *  - arch_vcpu_destroy() is called only if arch_vcpu_create() previously succeeded.
  */
-int  vcpu_initialise(struct vcpu *v);
-void vcpu_destroy(struct vcpu *v);
+int  arch_vcpu_create(struct vcpu *v);
+void arch_vcpu_destroy(struct vcpu *v);
 
 int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset);
 void unmap_vcpu_info(struct vcpu *v);
-- 
2.1.4


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

Re: [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency

[Jan Beulich]

>>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
> The vcpu functions are far less consistent than the domain side of things, and
> in particular, has vcpu_destroy() for architecture specific functionality.
> 
> Perform the following renames:
> 
>   * alloc_vcpu      => vcpu_create
>   * vcpu_initialise => arch_vcpu_create
>   * vcpu_destroy    => arch_vcpu_destroy
> 
> which makes the vcpu hierarchy consistent with the domain hierarchy.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency

[Roger Pau Monné]

On Thu, Sep 06, 2018 at 08:25:32PM +0100, Andrew Cooper wrote:
> The vcpu functions are far less consistent than the domain side of things, and
> in particular, has vcpu_destroy() for architecture specific functionality.
> 
> Perform the following renames:
> 
>   * alloc_vcpu      => vcpu_create
>   * vcpu_initialise => arch_vcpu_create
>   * vcpu_destroy    => arch_vcpu_destroy
> 
> which makes the vcpu hierarchy consistent with the domain hierarchy.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 5593495..6df6a58 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -13,7 +13,7 @@ typedef union {
>      struct compat_vcpu_guest_context *cmp;
>  } vcpu_guest_context_u __attribute__((__transparent_union__));
>  
> -struct vcpu *alloc_vcpu(
> +struct vcpu *vcpu_create(
>      struct domain *d, unsigned int vcpu_id, unsigned int cpu_id);
>  
>  unsigned int dom0_max_vcpus(void);
> @@ -47,13 +47,13 @@ void free_pirq_struct(void *);
>  
>  /*
>   * Initialise/destroy arch-specific details of a VCPU.
> - *  - vcpu_initialise() is called after the basic generic fields of the
> + *  - arch_vcpu_create() is called after the basic generic fields of the
>   *    VCPU structure are initialised. Many operations can be applied to the
>   *    VCPU at this point (e.g., vcpu_pause()).
> - *  - vcpu_destroy() is called only if vcpu_initialise() previously succeeded.
> + *  - arch_vcpu_destroy() is called only if arch_vcpu_create() previously succeeded.

Line is too long.

Thanks, Roger.

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

Re: [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency

[Julien Grall]

Hi,

On 09/06/2018 08:25 PM, Andrew Cooper wrote:
> The vcpu functions are far less consistent than the domain side of things, and
> in particular, has vcpu_destroy() for architecture specific functionality.
> 
> Perform the following renames:
> 
>    * alloc_vcpu      => vcpu_create
>    * vcpu_initialise => arch_vcpu_create
>    * vcpu_destroy    => arch_vcpu_destroy
> 
> which makes the vcpu hierarchy consistent with the domain hierarchy.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/domain.c       |  6 +++---
>   xen/arch/arm/domain_build.c |  4 ++--
>   xen/arch/x86/dom0_build.c   |  2 +-
>   xen/arch/x86/domain.c       |  4 ++--
>   xen/common/domain.c         |  6 +++---
>   xen/common/domctl.c         |  2 +-
>   xen/common/schedule.c       |  4 ++--
>   xen/include/xen/domain.h    | 10 +++++-----
>   8 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 4baecc2..feebbf5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -538,7 +538,7 @@ void free_vcpu_struct(struct vcpu *v)
>       free_xenheap_pages(v, get_order_from_bytes(sizeof(*v)));
>   }
>   
> -int vcpu_initialise(struct vcpu *v)
> +int arch_vcpu_create(struct vcpu *v)
>   {
>       int rc = 0;
>   
> @@ -583,11 +583,11 @@ int vcpu_initialise(struct vcpu *v)
>       return rc;
>   
>   fail:
> -    vcpu_destroy(v);
> +    arch_vcpu_destroy(v);
>       return rc;
>   }
>   
> -void vcpu_destroy(struct vcpu *v)
> +void arch_vcpu_destroy(struct vcpu *v)
>   {
>       vcpu_timer_destroy(v);
>       vcpu_vgic_free(v);
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2a383c8..5057ad8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -74,7 +74,7 @@ unsigned int __init dom0_max_vcpus(void)
>   
>   struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
>   {
> -    return alloc_vcpu(dom0, 0, 0);
> +    return vcpu_create(dom0, 0, 0);
>   }
>   
>   static unsigned int __init get_11_allocation_size(paddr_t size)
> @@ -2232,7 +2232,7 @@ int __init construct_dom0(struct domain *d)
>       for ( i = 1, cpu = 0; i < d->max_vcpus; i++ )
>       {
>           cpu = cpumask_cycle(cpu, &cpu_online_map);
> -        if ( alloc_vcpu(d, i, cpu) == NULL )
> +        if ( vcpu_create(d, i, cpu) == NULL )
>           {
>               printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu);
>               break;
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 423fdec..86eb7db 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -134,7 +134,7 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d,
>                                       unsigned int prev_cpu)
>   {
>       unsigned int cpu = cpumask_cycle(prev_cpu, &dom0_cpus);
> -    struct vcpu *v = alloc_vcpu(d, vcpu_id, cpu);
> +    struct vcpu *v = vcpu_create(d, vcpu_id, cpu);
>   
>       if ( v )
>       {
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 313ebb3..d67a047 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -322,7 +322,7 @@ void free_vcpu_struct(struct vcpu *v)
>       free_xenheap_page(v);
>   }
>   
> -int vcpu_initialise(struct vcpu *v)
> +int arch_vcpu_create(struct vcpu *v)
>   {
>       struct domain *d = v->domain;
>       int rc;
> @@ -382,7 +382,7 @@ int vcpu_initialise(struct vcpu *v)
>       return rc;
>   }
>   
> -void vcpu_destroy(struct vcpu *v)
> +void arch_vcpu_destroy(struct vcpu *v)
>   {
>       xfree(v->arch.vm_event);
>       v->arch.vm_event = NULL;
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 78c450e..14c8d00 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -123,7 +123,7 @@ static void vcpu_info_reset(struct vcpu *v)
>       v->vcpu_info_mfn = INVALID_MFN;
>   }
>   
> -struct vcpu *alloc_vcpu(
> +struct vcpu *vcpu_create(
>       struct domain *d, unsigned int vcpu_id, unsigned int cpu_id)
>   {
>       struct vcpu *v;
> @@ -165,7 +165,7 @@ struct vcpu *alloc_vcpu(
>       if ( sched_init_vcpu(v, cpu_id) != 0 )
>           goto fail_wq;
>   
> -    if ( vcpu_initialise(v) != 0 )
> +    if ( arch_vcpu_create(v) != 0 )
>       {
>           sched_destroy_vcpu(v);
>    fail_wq:
> @@ -870,7 +870,7 @@ static void complete_domain_destroy(struct rcu_head *head)
>           if ( (v = d->vcpu[i]) == NULL )
>               continue;
>           tasklet_kill(&v->continue_hypercall_tasklet);
> -        vcpu_destroy(v);
> +        arch_vcpu_destroy(v);
>           sched_destroy_vcpu(v);
>           destroy_waitqueue_vcpu(v);
>       }
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index ed047b7..2facbdb 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -571,7 +571,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                   cpumask_any(online) :
>                   cpumask_cycle(d->vcpu[i-1]->processor, online);
>   
> -            if ( alloc_vcpu(d, i, cpu) == NULL )
> +            if ( vcpu_create(d, i, cpu) == NULL )
>                   goto maxvcpu_out;
>           }
>   
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 05281d6..1ef5be9 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1645,7 +1645,7 @@ static int cpu_schedule_up(unsigned int cpu)
>           return 0;
>   
>       if ( idle_vcpu[cpu] == NULL )
> -        alloc_vcpu(idle_vcpu[0]->domain, cpu, cpu);
> +        vcpu_create(idle_vcpu[0]->domain, cpu, cpu);
>       else
>       {
>           struct vcpu *idle = idle_vcpu[cpu];
> @@ -1817,7 +1817,7 @@ void __init scheduler_init(void)
>       BUG_ON(IS_ERR(idle_domain));
>       idle_domain->vcpu = idle_vcpu;
>       idle_domain->max_vcpus = nr_cpu_ids;
> -    if ( alloc_vcpu(idle_domain, 0, 0) == NULL )
> +    if ( vcpu_create(idle_domain, 0, 0) == NULL )
>           BUG();
>       this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0);
>       BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv));
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 5593495..6df6a58 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -13,7 +13,7 @@ typedef union {
>       struct compat_vcpu_guest_context *cmp;
>   } vcpu_guest_context_u __attribute__((__transparent_union__));
>   
> -struct vcpu *alloc_vcpu(
> +struct vcpu *vcpu_create(
>       struct domain *d, unsigned int vcpu_id, unsigned int cpu_id);
>   
>   unsigned int dom0_max_vcpus(void);
> @@ -47,13 +47,13 @@ void free_pirq_struct(void *);
>   
>   /*
>    * Initialise/destroy arch-specific details of a VCPU.
> - *  - vcpu_initialise() is called after the basic generic fields of the
> + *  - arch_vcpu_create() is called after the basic generic fields of the
>    *    VCPU structure are initialised. Many operations can be applied to the
>    *    VCPU at this point (e.g., vcpu_pause()).
> - *  - vcpu_destroy() is called only if vcpu_initialise() previously succeeded.
> + *  - arch_vcpu_destroy() is called only if arch_vcpu_create() previously succeeded.
>    */
> -int  vcpu_initialise(struct vcpu *v);
> -void vcpu_destroy(struct vcpu *v);
> +int  arch_vcpu_create(struct vcpu *v);
> +void arch_vcpu_destroy(struct vcpu *v);
>   
>   int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset);
>   void unmap_vcpu_info(struct vcpu *v);
> 

-- 
Julien Grall

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

[PATCH 2/3] xen/vcpu: Introduce vcpu_destroy()

[Andrew Cooper]

Like _domain_destroy(), this will eventually idempotently free all parts of a
struct vcpu.

While breaking apart the failure path of vcpu_create(), rework the codeflow to
be in a line at the end of the function for clarity.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/domain.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 14c8d00..a9df589 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -123,6 +123,16 @@ static void vcpu_info_reset(struct vcpu *v)
     v->vcpu_info_mfn = INVALID_MFN;
 }
 
+static void vcpu_destroy(struct vcpu *v)
+{
+    free_cpumask_var(v->cpu_hard_affinity);
+    free_cpumask_var(v->cpu_hard_affinity_tmp);
+    free_cpumask_var(v->cpu_hard_affinity_saved);
+    free_cpumask_var(v->cpu_soft_affinity);
+
+    free_vcpu_struct(v);
+}
+
 struct vcpu *vcpu_create(
     struct domain *d, unsigned int vcpu_id, unsigned int cpu_id)
 {
@@ -147,7 +157,7 @@ struct vcpu *vcpu_create(
          !zalloc_cpumask_var(&v->cpu_hard_affinity_tmp) ||
          !zalloc_cpumask_var(&v->cpu_hard_affinity_saved) ||
          !zalloc_cpumask_var(&v->cpu_soft_affinity) )
-        goto fail_free;
+        goto fail;
 
     if ( is_idle_domain(d) )
     {
@@ -166,18 +176,7 @@ struct vcpu *vcpu_create(
         goto fail_wq;
 
     if ( arch_vcpu_create(v) != 0 )
-    {
-        sched_destroy_vcpu(v);
- fail_wq:
-        destroy_waitqueue_vcpu(v);
- fail_free:
-        free_cpumask_var(v->cpu_hard_affinity);
-        free_cpumask_var(v->cpu_hard_affinity_tmp);
-        free_cpumask_var(v->cpu_hard_affinity_saved);
-        free_cpumask_var(v->cpu_soft_affinity);
-        free_vcpu_struct(v);
-        return NULL;
-    }
+        goto fail_sched;
 
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )
@@ -197,6 +196,15 @@ struct vcpu *vcpu_create(
         domain_update_node_affinity(d);
 
     return v;
+
+ fail_sched:
+    sched_destroy_vcpu(v);
+ fail_wq:
+    destroy_waitqueue_vcpu(v);
+ fail:
+    vcpu_destroy(v);
+
+    return NULL;
 }
 
 static int late_hwdom_init(struct domain *d)
@@ -898,13 +906,7 @@ static void complete_domain_destroy(struct rcu_head *head)
 
     for ( i = d->max_vcpus - 1; i >= 0; i-- )
         if ( (v = d->vcpu[i]) != NULL )
-        {
-            free_cpumask_var(v->cpu_hard_affinity);
-            free_cpumask_var(v->cpu_hard_affinity_tmp);
-            free_cpumask_var(v->cpu_hard_affinity_saved);
-            free_cpumask_var(v->cpu_soft_affinity);
-            free_vcpu_struct(v);
-        }
+            vcpu_destroy(v);
 
     if ( d->target != NULL )
         put_domain(d->target);
-- 
2.1.4


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

Re: [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy()

[Jan Beulich]

>>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
> Like _domain_destroy(), this will eventually idempotently free all parts of a
> struct vcpu.
> 
> While breaking apart the failure path of vcpu_create(), rework the codeflow to
> be in a line at the end of the function for clarity.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy()

[Roger Pau Monné]

On Thu, Sep 06, 2018 at 08:25:33PM +0100, Andrew Cooper wrote:
> Like _domain_destroy(), this will eventually idempotently free all parts of a
> struct vcpu.
> 
> While breaking apart the failure path of vcpu_create(), rework the codeflow to
> be in a line at the end of the function for clarity.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

[PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Andrew Cooper]

Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
sanity BUG_ON().

Now that d->max_vcpus is appropriately set up before vcpu_create() is called,
we can properly range check the requested vcpu_id.  Drop the BUG_ON() and
replace it with code which is runtime safe but non-fatal.

While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
isn't a requirement for the threading the vcpu into the linked list, so update
the threading code to be more generic, and add a comment explaining why we
need to search for prev_id.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/setup.c |  1 -
 xen/arch/x86/setup.c |  1 -
 xen/common/domain.c  | 32 ++++++++++++++++++++++++++++----
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 01aaaab..d06ac40 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     set_processor_id(0); /* needed early, for smp_processor_id() */
 
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
-    idle_vcpu[0] = current;
 
     setup_virtual_regions(NULL, NULL);
     /* Initialize traps early allow us to get backtrace when an error occurred */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a2f22a1..5e1e8ae 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     set_processor_id(0);
     set_current(INVALID_VCPU); /* debug sanity. */
-    idle_vcpu[0] = current;
     init_shadow_spec_ctrl_state();
 
     percpu_init_areas();
diff --git a/xen/common/domain.c b/xen/common/domain.c
index a9df589..d23b54a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -138,7 +138,19 @@ struct vcpu *vcpu_create(
 {
     struct vcpu *v;
 
-    BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]);
+    /*
+     * Sanity check some input expectations:
+     *  - d->max_vcpus and d->vcpu[] should be set up
+     *  - vcpu_id should be bounded by d->max_vcpus
+     *  - v0 must be the first-allocated vcpu
+     *  - No previous vcpu with this id should be allocated
+     */
+    if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus ||
+         (vcpu_id > 0 && !d->vcpu[0]) || d->vcpu[vcpu_id] )
+    {
+        ASSERT_UNREACHABLE();
+        return NULL;
+    }
 
     if ( (v = alloc_vcpu_struct()) == NULL )
         return NULL;
@@ -178,15 +190,27 @@ struct vcpu *vcpu_create(
     if ( arch_vcpu_create(v) != 0 )
         goto fail_sched;
 
+    /* Insert the vcpu into the domain's vcpu list. */
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )
     {
         int prev_id = v->vcpu_id - 1;
+
+        /*
+         * Look for the previously allocated vcpu, and splice into the
+         * next_in_list single linked list.
+         *
+         * All domains other than IDLE have tightly packed vcpu_id's.  IDLE
+         * vcpu_id's are derived from hardware CPU id's and can be sparse.
+         */
         while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) )
             prev_id--;
-        BUG_ON(prev_id < 0);
-        v->next_in_list = d->vcpu[prev_id]->next_in_list;
-        d->vcpu[prev_id]->next_in_list = v;
+
+        if ( prev_id >= 0 )
+        {
+            v->next_in_list = d->vcpu[prev_id]->next_in_list;
+            d->vcpu[prev_id]->next_in_list = v;
+        }
     }
 
     /* Must be called after making new vcpu visible to for_each_vcpu(). */
-- 
2.1.4


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

Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Jason Andryuk]

On Thu, Sep 6, 2018 at 3:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
> idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
> sanity BUG_ON().
>
> Now that d->max_vcpus is appropriately set up before vcpu_create() is called,
> we can properly range check the requested vcpu_id.  Drop the BUG_ON() and
> replace it with code which is runtime safe but non-fatal.
>
> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
> isn't a requirement for the threading the vcpu into the linked list, so update
> the threading code to be more generic, and add a comment explaining why we
> need to search for prev_id.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/setup.c |  1 -
>  xen/arch/x86/setup.c |  1 -
>  xen/common/domain.c  | 32 ++++++++++++++++++++++++++++----
>  3 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 01aaaab..d06ac40 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>      set_processor_id(0); /* needed early, for smp_processor_id() */
>
>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
> -    idle_vcpu[0] = current;
>
>      setup_virtual_regions(NULL, NULL);
>      /* Initialize traps early allow us to get backtrace when an error occurred */
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a2f22a1..5e1e8ae 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
>      set_processor_id(0);
>      set_current(INVALID_VCPU); /* debug sanity. */
> -    idle_vcpu[0] = current;

xen/arch/x86/tboot.c:tboot_shutdown() has:
    if ( idle_vcpu[0] != INVALID_VCPU )
        write_ptbase(idle_vcpu[0]);

With your change, this should be update to check for non-NULL.

Regards,
Jason

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

Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Andrew Cooper]

On 06/09/18 21:07, Jason Andryuk wrote:
> On Thu, Sep 6, 2018 at 3:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
>> idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
>> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
>> sanity BUG_ON().
>>
>> Now that d->max_vcpus is appropriately set up before vcpu_create() is called,
>> we can properly range check the requested vcpu_id.  Drop the BUG_ON() and
>> replace it with code which is runtime safe but non-fatal.
>>
>> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
>> isn't a requirement for the threading the vcpu into the linked list, so update
>> the threading code to be more generic, and add a comment explaining why we
>> need to search for prev_id.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/setup.c |  1 -
>>  xen/arch/x86/setup.c |  1 -
>>  xen/common/domain.c  | 32 ++++++++++++++++++++++++++++----
>>  3 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 01aaaab..d06ac40 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      set_processor_id(0); /* needed early, for smp_processor_id() */
>>
>>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>> -    idle_vcpu[0] = current;
>>
>>      setup_virtual_regions(NULL, NULL);
>>      /* Initialize traps early allow us to get backtrace when an error occurred */
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index a2f22a1..5e1e8ae 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>
>>      set_processor_id(0);
>>      set_current(INVALID_VCPU); /* debug sanity. */
>> -    idle_vcpu[0] = current;
> xen/arch/x86/tboot.c:tboot_shutdown() has:
>     if ( idle_vcpu[0] != INVALID_VCPU )
>         write_ptbase(idle_vcpu[0]);
>
> With your change, this should be update to check for non-NULL.

Oh - very good catch.  Thanks.

Looking at the code, ideally it would change to
write_cr3(idle_pg_table), but that isn't going to include the additional
CR4 changes.  Leaving this in this form (with a NULL) check is probably
best.

~Andrew

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

Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Jan Beulich]

>>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
> isn't a requirement for the threading the vcpu into the linked list, so update
> the threading code to be more generic, and add a comment explaining why we
> need to search for prev_id.

I'm afraid I can't bring this in line with the code change:

> @@ -178,15 +190,27 @@ struct vcpu *vcpu_create(
>      if ( arch_vcpu_create(v) != 0 )
>          goto fail_sched;
>  
> +    /* Insert the vcpu into the domain's vcpu list. */
>      d->vcpu[vcpu_id] = v;
>      if ( vcpu_id != 0 )

There still is this conditional, and you don't add any else to it. Hence
afaics if vCPU 0 was created after vCPU 1, vCPU 0's next_in_list
would not be made point to vCPU 1. That's not what I'd call "more
generic".

But the question is what use the next_in_list field is in the first place,
when the entries there are sorted by ID anyway: Why can't we
simply use v->domain->vcpu[] instead? In the common case
v->domain->vcpu[v->vcpu_id+1] is not going to be NULL anyway,
and I don't think for_each_vcpu() would become that much more
complicated that way.

>      {
>          int prev_id = v->vcpu_id - 1;
> +
> +        /*
> +         * Look for the previously allocated vcpu, and splice into the
> +         * next_in_list single linked list.

I'm also not very happy about the use of "previously" here: This (to
me as a non-native speaker) in no way means "the one with the next
lowest ID".

Jan

> +         * All domains other than IDLE have tightly packed vcpu_id's.  IDLE
> +         * vcpu_id's are derived from hardware CPU id's and can be sparse.
> +         */
>          while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) )
>              prev_id--;
> -        BUG_ON(prev_id < 0);
> -        v->next_in_list = d->vcpu[prev_id]->next_in_list;
> -        d->vcpu[prev_id]->next_in_list = v;
> +
> +        if ( prev_id >= 0 )
> +        {
> +            v->next_in_list = d->vcpu[prev_id]->next_in_list;
> +            d->vcpu[prev_id]->next_in_list = v;
> +        }
>      }
>  
>      /* Must be called after making new vcpu visible to for_each_vcpu(). */
> -- 
> 2.1.4





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

Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Andrew Cooper]

On 07/09/18 11:15, Jan Beulich wrote:
>>>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
>> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
>> isn't a requirement for the threading the vcpu into the linked list, so update
>> the threading code to be more generic, and add a comment explaining why we
>> need to search for prev_id.
> I'm afraid I can't bring this in line with the code change:
>
>> @@ -178,15 +190,27 @@ struct vcpu *vcpu_create(
>>      if ( arch_vcpu_create(v) != 0 )
>>          goto fail_sched;
>>  
>> +    /* Insert the vcpu into the domain's vcpu list. */
>>      d->vcpu[vcpu_id] = v;
>>      if ( vcpu_id != 0 )
> There still is this conditional, and you don't add any else to it. Hence
> afaics if vCPU 0 was created after vCPU 1, vCPU 0's next_in_list
> would not be made point to vCPU 1. That's not what I'd call "more
> generic".

You are completely correct.  I don't know what I was thinking when
saying that this is more generic.

I suspect it also means that, depending on hardware CPU numbering,
for_each_vcpu(idle) may not work, but perhaps that is not an issue as
the idle domains pointer is actually private to scheduler_init().  It is
available from either current->domain, or idle_vcpu[$X]->domain, but I
think its reasonable to expect that we never use for_each_vcpu() over
the idle domain.

> But the question is what use the next_in_list field is in the first place,
> when the entries there are sorted by ID anyway: Why can't we
> simply use v->domain->vcpu[] instead? In the common case
> v->domain->vcpu[v->vcpu_id+1] is not going to be NULL anyway,
> and I don't think for_each_vcpu() would become that much more
> complicated that way.

It would, because every iteration you'd need a check of v->vcpu_id
against d->max_vcpus to avoid walking off the end of d->vcpu[]

>
>>      {
>>          int prev_id = v->vcpu_id - 1;
>> +
>> +        /*
>> +         * Look for the previously allocated vcpu, and splice into the
>> +         * next_in_list single linked list.
> I'm also not very happy about the use of "previously" here: This (to
> me as a non-native speaker) in no way means "the one with the next
> lowest ID".

Given the simplifying assumption of not the idle domain, the setting up
of the single linked list can be made much more concise, as we know the
IDs are packed and allocated in ascending order.

~Andrew

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

[PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create()

[Andrew Cooper]

Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
sanity BUG_ON().

Now that d->max_vcpus is appropriately set up before vcpu_create() is called,
we can properly range check the requested vcpu_id.  Drop the BUG_ON() and
replace it with code which is runtime safe but non-fatal.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Jason Andryuk <jandryuk@gmail.com>

v2:
 * Fix the tboot check following the un-poisioning of idle_vcpu[0]
 * Exclude the idle domain from the next_in_list list, and vastly simplify the
   linking logic.
---
 xen/arch/arm/setup.c |  1 -
 xen/arch/x86/setup.c |  1 -
 xen/arch/x86/tboot.c |  2 +-
 xen/common/domain.c  | 28 ++++++++++++++++++----------
 4 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ea2495a..a80032c 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     set_processor_id(0); /* needed early, for smp_processor_id() */
 
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
-    idle_vcpu[0] = current;
 
     setup_virtual_regions(NULL, NULL);
     /* Initialize traps early allow us to get backtrace when an error occurred */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2fbf7d5..2081592 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     set_processor_id(0);
     set_current(INVALID_VCPU); /* debug sanity. */
-    idle_vcpu[0] = current;
     init_shadow_spec_ctrl_state();
 
     percpu_init_areas();
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index f3fdee4..fff00bf 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -395,7 +395,7 @@ void tboot_shutdown(uint32_t shutdown_type)
      * During early boot, we can be called by panic before idle_vcpu[0] is
      * setup, but in that case we don't need to change page tables.
      */
-    if ( idle_vcpu[0] != INVALID_VCPU )
+    if ( idle_vcpu[0] )
         write_ptbase(idle_vcpu[0]);
 
     ((void(*)(void))(unsigned long)g_tboot_shared->shutdown_entry)();
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 66054b3..a0d950c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -138,7 +138,21 @@ struct vcpu *vcpu_create(
 {
     struct vcpu *v;
 
-    BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]);
+    /*
+     * Sanity check some input expectations:
+     *  - d->max_vcpus and d->vcpu[] should be set up
+     *  - vcpu_id should be bounded by d->max_vcpus
+     *  - Vcpus should be tightly packed and allocated in ascending order
+     *    (except for the idle domain).
+     *  - No previous vcpu with this id should be allocated
+     */
+    if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus ||
+         (!is_idle_domain(d) && vcpu_id && !d->vcpu[vcpu_id - 1]) ||
+         d->vcpu[vcpu_id] )
+    {
+        ASSERT_UNREACHABLE();
+        return NULL;
+    }
 
     if ( (v = alloc_vcpu_struct()) == NULL )
         return NULL;
@@ -178,16 +192,10 @@ struct vcpu *vcpu_create(
     if ( arch_vcpu_create(v) != 0 )
         goto fail_sched;
 
+    /* Insert the vcpu into the domain's vcpu list. */
     d->vcpu[vcpu_id] = v;
-    if ( vcpu_id != 0 )
-    {
-        int prev_id = v->vcpu_id - 1;
-        while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) )
-            prev_id--;
-        BUG_ON(prev_id < 0);
-        v->next_in_list = d->vcpu[prev_id]->next_in_list;
-        d->vcpu[prev_id]->next_in_list = v;
-    }
+    if ( !is_idle_domain(d) && vcpu_id > 0 )
+        d->vcpu[vcpu_id - 1]->next_in_list = v;
 
     /* Must be called after making new vcpu visible to for_each_vcpu(). */
     vcpu_check_shutdown(v);
-- 
2.1.4


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

Re: [PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create()

[Jason Andryuk]

On Tue, Sep 11, 2018 at 12:48 PM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
> idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
> sanity BUG_ON().
>
> Now that d->max_vcpus is appropriately set up before vcpu_create() is called,
> we can properly range check the requested vcpu_id.  Drop the BUG_ON() and
> replace it with code which is runtime safe but non-fatal.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Jason Andryuk <jandryuk@gmail.com>
>
> v2:
>  * Fix the tboot check following the un-poisioning of idle_vcpu[0]
>  * Exclude the idle domain from the next_in_list list, and vastly simplify the
>    linking logic.

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

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

Re: [PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create()

[Jan Beulich]

>>> On 11.09.18 at 18:46, <andrew.cooper3@citrix.com> wrote:
> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
> idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
> sanity BUG_ON().

But you completely discount the intended effect of this poisoning:
During early boot, a NULL deref is liable to not fault, while a deref
of INVALID_VCPU is always going to (on x86 at least).

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -138,7 +138,21 @@ struct vcpu *vcpu_create(

This patch does not look to be based on current staging.

>  {
>      struct vcpu *v;
>  
> -    BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]);
> +    /*
> +     * Sanity check some input expectations:
> +     *  - d->max_vcpus and d->vcpu[] should be set up
> +     *  - vcpu_id should be bounded by d->max_vcpus
> +     *  - Vcpus should be tightly packed and allocated in ascending order
> +     *    (except for the idle domain).
> +     *  - No previous vcpu with this id should be allocated
> +     */
> +    if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus ||
> +         (!is_idle_domain(d) && vcpu_id && !d->vcpu[vcpu_id - 1]) ||

Note how you still require an is_idle_domain() special case here
anyway.

> +         d->vcpu[vcpu_id] )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return NULL;

I assume you consider it acceptable for release builds to report
back -ENOMEM in the (hopefully indeed impossible) case of
execution going this path?

> @@ -178,16 +192,10 @@ struct vcpu *vcpu_create(
>      if ( arch_vcpu_create(v) != 0 )
>          goto fail_sched;
>  
> +    /* Insert the vcpu into the domain's vcpu list. */
>      d->vcpu[vcpu_id] = v;
> -    if ( vcpu_id != 0 )
> -    {
> -        int prev_id = v->vcpu_id - 1;
> -        while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) )
> -            prev_id--;
> -        BUG_ON(prev_id < 0);
> -        v->next_in_list = d->vcpu[prev_id]->next_in_list;
> -        d->vcpu[prev_id]->next_in_list = v;
> -    }
> +    if ( !is_idle_domain(d) && vcpu_id > 0 )
> +        d->vcpu[vcpu_id - 1]->next_in_list = v;

While before this change for_each_vcpu(idle_domain) was
broken only for the (currently impossible on x86 at least) case
of CPU0 not being online (nor parked), afaict it will now be
broken altogether, leading to NULL deref-s when used. Is that
really what you want (if so, the description should say so, and
why that's okay)?

Jan



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