Re: [Xen-devel] [PATCH v3 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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