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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
