Re: [Xen-devel] [PATCH v3 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

[Eric DeVolder <eric.devolder@xxxxxxxxxx>]

On 04/19/2017 10:47 AM, Eric DeVolder wrote:
> The spinlock in kexec_swap_images() was removed as
> this function is only reachable on the kexec hypercall, which is
> now protected at the top-level in do_kexec_op_internal(),
> thus the local spinlock is no longer necessary.
>
> Per recommendation from Jan Beulich and Andrew Cooper, I left
> an ASSERT in place of the spin_lock().
>
> Signed-off-by: Eric DeVolder <eric.devolder@xxxxxxxxxx>
> Reviewed-by: Bhavesh Davda <bhavesh.davda@xxxxxxxxxx>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
> v3:
>  - Incorporated feedback from Jan Beulich and Andrew Cooper
>    to leave an ASSERT where spin_lock() once was.
>
> v2: 04/17/2017
>  - Patch titled 'kexec: use hypercall_create_continuation to protect KEXEC ops'
>  - Separated removal of spinlock in kexec_swap_images() into its own patch.
>
> v1: 04/10/2017
>  - Patch titled 'kexec: Add spinlock for the whole hypercall'
>  - Removal of spinlock in kexec_swap_images() was part of other patch.
> ---
>  xen/common/kexec.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/xen/common/kexec.c b/xen/common/kexec.c
> index 253c204..af32e58 100644
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -820,7 +820,6 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
>  static int kexec_swap_images(int type, struct kexec_image *new,
>                               struct kexec_image **old)
>  {
> -    static DEFINE_SPINLOCK(kexec_lock);
>      int base, bit, pos;
>      int new_slot, old_slot;
>
> @@ -832,7 +831,7 @@ static int kexec_swap_images(int type, struct kexec_image 
> *new,
>      if ( kexec_load_get_bits(type, &base, &bit) )
>          return -EINVAL;
>
> -    spin_lock(&kexec_lock);
> +    ASSERT( !test_bit(KEXEC_FLAG_IN_HYPERCALL, &kexec_flags) );
There are two problems here: the spaces need to be eliminated, and more 
importantly, the sense/polarity of the expression is wrong, the '!' 
needs to be removed.
>      pos = (test_bit(bit, &kexec_flags) != 0);
>      old_slot = base + pos;
> @@ -846,8 +845,6 @@ static int kexec_swap_images(int type, struct kexec_image 
> *new,
>      clear_bit(old_slot, &kexec_flags);
>      *old = kexec_image[old_slot];
>
> -    spin_unlock(&kexec_lock);
> -
>      return 0;
>  }

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