Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/12/2019 12:57, Julien Grall wrote:
> Hi Andrew,
>
> On 05/12/2019 22:30, Andrew Cooper wrote:
>> These hypercalls each use continue_hypercall_on_cpu(), whose API is
>> about to
>> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
>> folding existing contination logic where applicable.
>>
>> In addition:
>>   * For platform op and sysctl, insert a cpu_relax() into what is
>> otherwise a
>>     tight spinlock loop, and make the continuation logic common at the
>>     epilogue.
>>   * Contrary to the comment in the code, kexec_exec() does return in the
>>     KEXEC_REBOOT case, needs to pass ret back to the caller.
>
> It is not entirely trivial to me that KEXEC_REBOOT refers to
> KEXEC_DEFAULT_TYPE. The more that if you look at the kexec_reboot()
> helper, it will not return (see BUG()). What may return is
> continue_hypercall_on_cpu().
>
> So would it make sense to use KEXEC_DEFAULT_TYPE?

I'm not sure why I capitalised it, but no - using KEXEC_DEFAULT_TYPE is
worse.  A casual reader is far more likely to understand kexec_reboot()
in this context.

>
> [...]
>
>> @@ -816,6 +819,13 @@ ret_t
>> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>>    out:
>>       spin_unlock(&xenpf_lock);
>>   +    if ( ret == -ERESTART )
>> +    {
>> +    create_continuation:
>
> Shall we indent create_continuation the same way as out?

They have different scopes, and while it may look weird, this is in
accordance with our style.

>
> [...]
>
>> @@ -1263,13 +1263,25 @@ static int do_kexec_op_internal(unsigned long
>> op,
>>     long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
>> uarg)
>>   {
>> -    return do_kexec_op_internal(op, uarg, 0);
>> +    int ret = do_kexec_op_internal(op, uarg, 0);
> Shouldn't it be long (or unsigned long) here? Otherwise, the return of
> hypercall_create_continuation() may be truncated.

If you're concerned about truncation via this pattern, then there are
other areas of the code to be worred about.

However, there is nothing to truncate.  The return value of
hypercall_create_continuation() is the primary hypercall number, i.e.
__HYPERVISOR_kexec_op in this case.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel