[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] kexec: Add spinlock for the whole hypercall
>>> 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). If such a global locking approach is really necessary (i.e. if we 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? Furthermore - are there problems also with e.g. loading a default and a crash kernel in parallel? I.e. aren't you doing more synchronization than really necessary? Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |