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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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
might not be other places to _also_ add ASSERT()s, but not in _that
other_ patch.

Jan


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