[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops
On Thu, Apr 20, 2017 at 03:34:21AM -0600, Jan Beulich wrote: > >>> On 19.04.17 at 19:16, <daniel.kiper@xxxxxxxxxx> wrote: > > On Wed, Apr 19, 2017 at 10:19:44AM -0600, Jan Beulich wrote: > >> >>> On 19.04.17 at 17:54, <daniel.kiper@xxxxxxxxxx> wrote: > >> > On Wed, Apr 19, 2017 at 10:47:15AM -0500, Eric DeVolder wrote: > >> >> @@ -1193,6 +1194,9 @@ static int do_kexec_op_internal(unsigned long op, > >> >> if ( ret ) > >> >> return ret; > >> >> > >> >> + if ( test_and_set_bit(KEXEC_FLAG_IN_HYPERCALL, &kexec_flags) ) > >> >> + return hypercall_create_continuation(__HYPERVISOR_kexec_op, > >> >> "lh", op, uarg); > >> >> + > >> > > >> > I would suggest here: > >> > ASSERT(test_bit(KEXEC_FLAG_IN_HYPERCALL, &kexec_flags)); > >> > >> You're kidding? The flag was set just in the line above. Or do you > >> really mean we need to consider test_and_set_bit() not doing what > >> it is supposed to do? > > > > Yep, it looks ridiculous. However, ASSERT() in kexec_swap_images() looks > > almost > > the same for me. So, TBH, I still do not understand need for it at all. > > Could > > you enlighten me? > > Can't be that difficult to understand: There was a lock there before, > and the addition of the ASSERT() could help document that the > serialization requirements aren't being broken. I'm not saying there Ahhh... OK, so, you treat this more as a comment than anything else. So, why not use regular comment here then? Hmmm... Do you treat an ASSERT() here as kind of fuse too? > might not be other places to _also_ add ASSERT()s, but not in _that > other_ patch. OK, so, I would put ASSERT() at least at the beginning of kexec_load() and kexec_unload() (I am not sure about others). However, then ASSERT() is not needed in kexec_swap_images(). Though if you wish to leave it I will not object any longer. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |