[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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
   * 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

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)
   +    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
     long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
-    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).


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.