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

[Julien Grall <julien@xxxxxxx>]

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?

[...]

> @@ -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?

[...]

> @@ -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 ( ret == -ERESTART )
> +        ret = hypercall_create_continuation(__HYPERVISOR_kexec_op,
> +                                            "lh", op, uarg);
> +
> +    return ret;
>   }
[...]

> @@ -516,6 +519,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>            __copy_to_guest(u_sysctl, op, 1) )
>           ret = -EFAULT;
>   
> +    if ( ret == -ERESTART )
> +    {
> +    create_continuation:

Similar question as the previous label create_continuation.

> +        ret = hypercall_create_continuation(__HYPERVISOR_sysctl, "h", 
> u_sysctl);
> +    }
> +
>       return ret;
>   }
>   
--
Julien Grall

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