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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 09/12/2019 17:37, Andrew Cooper wrote:
> 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.
But kexec_reboot() cannot return (see BUG()) so a reader may not 
understand why you suggest that it will return. So I think this want to 
be reworded.
> > [...]
> >
> > > @@ -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.
I knew you would mention the other places :).

> 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.
Make sense. And I guess the signed bit will be propagated so if 
do_kexec_op() return -EINVAL (32-bit), then it would still be -EINVAL 
(64-bit).
Cheers,

--
Julien Grall

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