Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[David Vrabel <david.vrabel@citrix.com>]

On 06/11/13 12:22, Jan Beulich wrote:
> This doesn't appear to have served any purpose other than causing
> map_pages_to_xen() to be (incorrectly) invoked with interrupts
> disabled. In particular, serialization against actual kexec-ing is done
> without this lock being involved. Clarify the scope of the lock at once
> by making it local to do_kexec_op_internal().
> 
> Once at it, also drop a pointless initializer.

I've already dropped the irqsave from this lock in my kexec series and
can fold the other part in as well (which would now be in
kexec_swap_images()).

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -55,8 +55,6 @@ static xen_kexec_image_t kexec_image[KEX
>  
>  static unsigned long kexec_flags = 0; /* the lowest bits are for KEXEC_IMAGE... */
>  
> -static spinlock_t kexec_lock = SPIN_LOCK_UNLOCKED;
> -
>  static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES];
>  static size_t vmcoreinfo_size = 0;
>  
> @@ -851,10 +849,9 @@ static int do_kexec_op_internal(unsigned
>                                  XEN_GUEST_HANDLE_PARAM(void) uarg,
>                                  bool_t compat)
>  {
> -    unsigned long flags;
> -    int ret = -EINVAL;
> +    static DEFINE_SPINLOCK(kexec_lock);
> +    int ret = xsm_kexec(XSM_PRIV);
>  
> -    ret = xsm_kexec(XSM_PRIV);
>      if ( ret )
>          return ret;

I find the resulting spacing of this a bit unconventional.

    int ret;

    ret = xsm_kexec(XSM_PRIV)
    if ( ret )
        return ret;

is more standard I think.  Not too bothered though.

I'm about to post v10 of the kexec series and it would be less hassle
for me if you took the fix via that series instead since this area
undergoes significant refactoring.  But if you prefer to commit this fix
now:

Acked-by: David Vrabel <david.vrabel@citrix.com>

David