[PATCH] xen/domctl: Drop vcpu_alloc_lock

[PATCH] xen/domctl: Drop vcpu_alloc_lock

[Andrew Cooper]

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@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 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);
 
-- 
2.1.4


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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[George Dunlap]

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

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

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[Wei Liu]

On Wed, Sep 05, 2018 at 08:15:25PM +0100, 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@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[Jan Beulich]

>>> On 05.09.18 at 21:15, <andrew.cooper3@citrix.com> 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.

The interlock didn't work as intended, I agree, but "serves no purpose"
is wrong imo. Rather than blindly dropping the logic, I'd have expected
for it to be fixed: Despite the movement into XEN_DOMCTL_createdomain
there's still a race between ucode updates and domain creation.

I see you've rushed the patch in (perhaps to avoid objections, given
that you've proposed this removal before, and I didn't really like it),
so I guess we need to take it from there now. I certainly agree that
runtime ucode updates aren't in good shape anyway, but I certainly
don't think making a bad situation worse really helps. In any event -
I would have expected you to give a little more time between patch
submission and patch committing, especially when the subject was
previously controversial, no matter that you've has the formally
necessary ack-s (which, if you had pointed out the previous
controversy, might not have been given that easily/quickly).

FAOD I'm not suggesting to revert the patch at this point in time.

Jan



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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[Andrew Cooper]

On 07/09/18 09:48, Jan Beulich wrote:
>>>> On 05.09.18 at 21:15, <andrew.cooper3@citrix.com> 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.

> I see you've rushed the patch in (perhaps to avoid objections, given
> that you've proposed this removal before, and I didn't really like it),
> so I guess we need to take it from there now. 

There was nothing deliberate here.  TBH, I thought the patch had been
pending on the list for longer than it had.  Either way, as we are
starting the conversation again...

> The interlock didn't work as intended, I agree, but "serves no purpose"
> is wrong imo.

At the moment, I stand my by statement, because as far as I can tell,
the interlock literally does nothing.

> Rather than blindly dropping the logic, I'd have expected
> for it to be fixed: Despite the movement into XEN_DOMCTL_createdomain
> there's still a race between ucode updates and domain creation.

What race?  What have I overlooked?

~Andrew

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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[Jan Beulich]

>>> On 07.09.18 at 11:57, <andrew.cooper3@citrix.com> wrote:
> On 07/09/18 09:48, Jan Beulich wrote:
>> Rather than blindly dropping the logic, I'd have expected
>> for it to be fixed: Despite the movement into XEN_DOMCTL_createdomain
>> there's still a race between ucode updates and domain creation.
> 
> What race?  What have I overlooked?

One CPU doing a microcode update while the other creates a
domain. I haven't looked in detail, but I think all domain creation
should be deferred until completion of the microcode update, so
that domains get a consistent/predictable OSVW state set up.

Of course we can put ourselves on the position that it is an
admin mistake to invoke a microcode update without suspending
domain creations temporarily, but since domain creation includes
domain reboots, I don't think this is something an admin can
fully control.

Yet further we could put ourselves on the position that ucode
updates with _any_ guests running are a bad idea.

Jan



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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[Andrew Cooper]

On 07/09/18 11:21, Jan Beulich wrote:
>>>> On 07.09.18 at 11:57, <andrew.cooper3@citrix.com> wrote:
>> On 07/09/18 09:48, Jan Beulich wrote:
>>> Rather than blindly dropping the logic, I'd have expected
>>> for it to be fixed: Despite the movement into XEN_DOMCTL_createdomain
>>> there's still a race between ucode updates and domain creation.
>> What race?  What have I overlooked?
> One CPU doing a microcode update while the other creates a
> domain. I haven't looked in detail, but I think all domain creation
> should be deferred until completion of the microcode update, so
> that domains get a consistent/predictable OSVW state set up.

The path in domain create will see a consistent OSVW, which will be
either the pre or post microcode value.  The other aspect (which again
can't be covered with an interlock like this) is that nothing goes and
adjusts the already-created domains to update their OSVW values.

> Of course we can put ourselves on the position that it is an
> admin mistake to invoke a microcode update without suspending
> domain creations temporarily, but since domain creation includes
> domain reboots, I don't think this is something an admin can
> fully control.

Domain reboots (including soft reset) are an entirely
toolstack-controlled actions.  Xen raises VIRQ_DOM_EXC and does nothing
more with the domain.  The toolstack subsequently issues a new
DOMCTL_createdomain.

libxl's architecture may make it hard to defer rebooting for a short
period, but such a design doesn't impact other toolstacks.

> Yet further we could put ourselves on the position that ucode
> updates with _any_ guests running are a bad idea.

Boot time microcode loading is by far the best option going (and with
live migration, there is little excuse to avoid using this method).

Runtime microcode loading has a few real bugs which need addressing, but
fundamentally, the list of things an admin needs to consider is massive.
To start with, is this piece of ucode even safe to runtime patch? (There
is at least one piece of ucode from each vendor where the answer to this
is a definite no.)

~Andrew

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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[George Dunlap]

On 09/07/2018 11:59 AM, Andrew Cooper wrote:
> On 07/09/18 11:21, Jan Beulich wrote:
>>>>> On 07.09.18 at 11:57, <andrew.cooper3@citrix.com> wrote:
>>> On 07/09/18 09:48, Jan Beulich wrote:
>>>> Rather than blindly dropping the logic, I'd have expected
>>>> for it to be fixed: Despite the movement into XEN_DOMCTL_createdomain
>>>> there's still a race between ucode updates and domain creation.
>>> What race?  What have I overlooked?
>> One CPU doing a microcode update while the other creates a
>> domain. I haven't looked in detail, but I think all domain creation
>> should be deferred until completion of the microcode update, so
>> that domains get a consistent/predictable OSVW state set up.
> 
> The path in domain create will see a consistent OSVW, which will be
> either the pre or post microcode value.  The other aspect (which again
> can't be covered with an interlock like this) is that nothing goes and
> adjusts the already-created domains to update their OSVW values.
> 
>> Of course we can put ourselves on the position that it is an
>> admin mistake to invoke a microcode update without suspending
>> domain creations temporarily, but since domain creation includes
>> domain reboots, I don't think this is something an admin can
>> fully control.
> 
> Domain reboots (including soft reset) are an entirely
> toolstack-controlled actions.  Xen raises VIRQ_DOM_EXC and does nothing
> more with the domain.  The toolstack subsequently issues a new
> DOMCTL_createdomain.
> 
> libxl's architecture may make it hard to defer rebooting for a short
> period, but such a design doesn't impact other toolstacks.

But if we do need some sort of interlock, Xen is the one that "knows"
about the rule, so Xen should be the one to enforce it.  Expecting the
toolstack to "remember" not to do any domain operations while doing
ucode update is going to be fragile.

 -George

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

Re: [PATCH] xen/domctl: Drop vcpu_alloc_lock

[Andrew Cooper]

On 07/09/18 13:37, George Dunlap wrote:
> On 09/07/2018 11:59 AM, Andrew Cooper wrote:
>> On 07/09/18 11:21, Jan Beulich wrote:
>>>>>> On 07.09.18 at 11:57, <andrew.cooper3@citrix.com> wrote:
>>>> On 07/09/18 09:48, Jan Beulich wrote:
>>>>> Rather than blindly dropping the logic, I'd have expected
>>>>> for it to be fixed: Despite the movement into XEN_DOMCTL_createdomain
>>>>> there's still a race between ucode updates and domain creation.
>>>> What race?  What have I overlooked?
>>> One CPU doing a microcode update while the other creates a
>>> domain. I haven't looked in detail, but I think all domain creation
>>> should be deferred until completion of the microcode update, so
>>> that domains get a consistent/predictable OSVW state set up.
>> The path in domain create will see a consistent OSVW, which will be
>> either the pre or post microcode value.  The other aspect (which again
>> can't be covered with an interlock like this) is that nothing goes and
>> adjusts the already-created domains to update their OSVW values.
>>
>>> Of course we can put ourselves on the position that it is an
>>> admin mistake to invoke a microcode update without suspending
>>> domain creations temporarily, but since domain creation includes
>>> domain reboots, I don't think this is something an admin can
>>> fully control.
>> Domain reboots (including soft reset) are an entirely
>> toolstack-controlled actions.  Xen raises VIRQ_DOM_EXC and does nothing
>> more with the domain.  The toolstack subsequently issues a new
>> DOMCTL_createdomain.
>>
>> libxl's architecture may make it hard to defer rebooting for a short
>> period, but such a design doesn't impact other toolstacks.
> But if we do need some sort of interlock, Xen is the one that "knows"
> about the rule, so Xen should be the one to enforce it.  Expecting the
> toolstack to "remember" not to do any domain operations while doing
> ucode update is going to be fragile.

Right, but

a) (I assert) We don't need an interlock to being with

b) Nothing we can implement in Xen check any of the boxes on the "Admin
must know what they are doing systemwide, including policy decisions of
if/when/how to adjust the already-running VMs" list.

~Andrew

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