Re: [Xen-devel] [PATCH] kexec: Add spinlock for the whole hypercall

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Apr 11, 2017 at 01:46:37AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 21:44, <eric.devolder@xxxxxxxxxx> wrote:
>
> Please don't forget Cc-ing the maintainer(s) of the code being modified.
>
> > @@ -1187,12 +1182,22 @@ static int do_kexec_op_internal(unsigned long op,
> >                                  XEN_GUEST_HANDLE_PARAM(void) uarg,
> >                                  bool_t compat)
> >  {
> > +    static DEFINE_SPINLOCK(kexec_lock);
> >      int ret = -EINVAL;
> >
> >      ret = xsm_kexec(XSM_PRIV);
> >      if ( ret )
> >          return ret;
> >
> > +    /*
> > +     * Because we write directly to the reserved memory
> > +     * region when loading crash kernels we need a spinlock here to
> > +     * prevent multiple crash kernels from attempting to load
> > +     * simultaneously, and to prevent a crash kernel from loading
> > +     * over the top of a in use crash kernel.
> > +     */
> > +    spin_lock(&kexec_lock);
> > +
> >      switch ( op )
> >      {
> >      case KEXEC_CMD_kexec_get_range:
> > @@ -1227,6 +1232,8 @@ static int do_kexec_op_internal(unsigned long op,
> >          break;
> >      }
> >
> > +    spin_unlock(&kexec_lock);
> > +
> >      return ret;
> >  }
>
> How long can a kexec-op take? Are you putting systems at risk of
> the watchdog firing? Even if you don't, you put all sorts of
> operations inside a lock now which preferably should run outside
> of atomic context (memory allocation, guest memory accesses).

Facepalm! I forgot about that.

> If such a global locking approach is really necessary (i.e. if we

Potentially no but otherwise we will further complicate sufficiently
complicated code now. So, I would like to have one sync place.

> can't expect the - privileged - caller to synchronize calls properly),
> wouldn't it be better to handle this with a static state variable,
> which gets checked/set atomically, and which if already set simply
> leads to a continuation being arranged for?

Do you think about something like that:

if ( test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags) && 
hypercall_preempt_check() )
    return hypercall_create_continuation(__HYPERVISOR_kexec_op, "h", uarg);

> Furthermore - are there problems also with e.g. loading a
> default and a crash kernel in parallel? I.e. aren't you doing more

No it should not be any issue there.

> synchronization than really necessary?

I do not think that it is very big issue here but if you wish we can fix it 
that too.

Daniel

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