Re: [Xen-devel] [PATCH] xen/domctl: Drop vcpu_alloc_lock

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/05/2018 08:15 PM, Andrew Cooper wrote:
> Since its introduction in c/s 8cbb5278e "x86/AMD: Add support for AMD's OSVW
> feature in guests", the OSVW data has been corrected to be per-domain rather
> than per-vcpu, and is initialised during XEN_DOMCTL_createdomain.
> 
> Furthermore, because XENPF_microcode_update uses hypercall continuations to
> move between CPUs, it drops the vcpu_alloc_lock mid update, meaning that it
> didn't provided the interlock guarantee that the OSVW patch was looking for in
> the first place.
> 
> This interlock serves no purpose, so take the opportunity to drop it and
> remove a global spinlock from the hypervisor.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/x86/platform_hypercall.c | 15 ---------------
>  xen/common/domctl.c               | 18 ------------------
>  xen/include/xen/domain.h          |  1 -
>  3 files changed, 34 deletions(-)
> 
> diff --git a/xen/arch/x86/platform_hypercall.c 
> b/xen/arch/x86/platform_hypercall.c
> index ea18c32..b19f6ec 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -280,24 +280,9 @@ ret_t 
> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>  
>          guest_from_compat_handle(data, op->u.microcode.data);
>  
> -        /*
> -         * alloc_vcpu() will access data which is modified during
> -         * microcode update
> -         */
> -        while ( !spin_trylock(&vcpu_alloc_lock) )
> -        {
> -            if ( hypercall_preempt_check() )
> -            {
> -                ret = hypercall_create_continuation(
> -                    __HYPERVISOR_platform_op, "h", u_xenpf_op);
> -                goto out;
> -            }
> -        }
> -
>          ret = microcode_update(
>                  guest_handle_to_param(data, const_void),
>                  op->u.microcode.length);
> -        spin_unlock(&vcpu_alloc_lock);
>      }
>      break;
>  
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index ee0983d..ed047b7 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -34,7 +34,6 @@
>  #include <xsm/xsm.h>
>  
>  static DEFINE_SPINLOCK(domctl_lock);
> -DEFINE_SPINLOCK(vcpu_alloc_lock);
>  
>  static int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
>                                     const unsigned long *bitmap,
> @@ -560,20 +559,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          /* Needed, for example, to ensure writable p.t. state is synced. */
>          domain_pause(d);
>  
> -        /*
> -         * Certain operations (e.g. CPU microcode updates) modify data which 
> is
> -         * used during VCPU allocation/initialization
> -         */
> -        while ( !spin_trylock(&vcpu_alloc_lock) )
> -        {
> -            if ( hypercall_preempt_check() )
> -            {
> -                ret =  hypercall_create_continuation(
> -                    __HYPERVISOR_domctl, "h", u_domctl);
> -                goto maxvcpu_out_novcpulock;
> -            }
> -        }
> -
>          ret = -ENOMEM;
>          online = cpupool_domain_cpumask(d);
>  
> @@ -593,9 +578,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          ret = 0;
>  
>      maxvcpu_out:
> -        spin_unlock(&vcpu_alloc_lock);
> -
> -    maxvcpu_out_novcpulock:
>          domain_unpause(d);
>          break;
>      }
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index ce31999..5593495 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -87,7 +87,6 @@ void arch_dump_domain_info(struct domain *d);
>  
>  int arch_vcpu_reset(struct vcpu *);
>  
> -extern spinlock_t vcpu_alloc_lock;
>  bool_t domctl_lock_acquire(void);
>  void domctl_lock_release(void);
>  
> 


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