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

Re: [PATCH 03/12] xen: harmonize return types of hypercall handlers

On 18.10.21 13:55, Jan Beulich wrote:
On 15.10.2021 14:51, Juergen Gross wrote:
Today most hypercall handlers have a return type of long, while the
compat ones return an int. There are a few exceptions from that rule,

Get rid of the exceptions by letting compat handlers always return int
and others always return long.

For the compat hvm case use eax instead of rax for the stored result as
it should have been from the beginning.

Additionally move some prototypes to include/asm-x86/hypercall.h
as they are x86 specific. Move the do_physdev_op() prototype from both
architecture dependant headers to the common one. Move the
compat_platform_op() prototype to the common header.

Switch some non style compliant types (u32, s32, s64) to style compliant

Rename paging_domctl_continuation() to do_paging_domctl_cont() and add
a matching define for the associated hypercall.

Make do_callback_op() and compat_callback_op() more similar by adding
the const attribute to compat_callback_op()'s 2nd parameter.

The do_platform_op() prototype needs to be modified in order to better
match its compat variant.

"Better" in what direction? So far both have been using typed handles,
which I consider better than void ones. You also don't seem to have
had a reason to switch e.g. multicall or dm_op, where (different)
typed handles are also in use. So I wonder what the reason for this
change is.

I had some problems making this build. Before my patches
platform_hypercall.c didn't include the header with the prototype,
so there was no mismatch detected.

Thanks for the pointers above. dm_op is no good example, as the
compat variant is explicitly a different implementation compared
to the normal one. But with the multicall example I can have
another try converting platform_op to a type safe variant using
non-void handles.

Change the type of the cmd parameter for [do|compat]_kexec_op() to
unsigned int, as this is more appropriate for the compat case.

The change for the compat case is fine, but for native you change
behavior for callers passing values equaling valid KEXEC_CMD_*
modulo 2³².

TBH, I don't think this is really a problem. Or do you think there
really is a user of this interface relying on a -ENOSYS in this

--- a/xen/arch/x86/pv/misc-hypercalls.c
+++ b/xen/arch/x86/pv/misc-hypercalls.c
@@ -28,12 +28,16 @@ long do_set_debugreg(int reg, unsigned long value)
      return set_debugreg(current, reg, value);
-unsigned long do_get_debugreg(int reg)
+long do_get_debugreg(int reg)
-    unsigned long val;
-    int res = x86emul_read_dr(reg, &val, NULL);
-    return res == X86EMUL_OKAY ? val : -ENODEV;
+    /* Avoid undefined behavior due to casting an unsigned long to long. */

Nit: unsigned -> signed conversion is implementation-defined, not

Okay, will change the comment.

--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -2207,13 +2207,13 @@ do_argo_op(unsigned int cmd, 
-compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
-               XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
-               unsigned long arg4)
+int compat_argo_op(unsigned int cmd,
+                   XEN_GUEST_HANDLE_PARAM(void) arg1,
+                   XEN_GUEST_HANDLE_PARAM(void) arg2,
+                   unsigned long arg3, unsigned long arg4)

Strictly speaking arg3 and arg4 also ought to be unsigned int here.
But that's perhaps for a separate patch at another time.

I'd rather leave it as is, as this way I can use the same definition for
both cases in patch 6. And I don't see how anything could go wrong this
way, as expanding a 32-bit unsigned value to 64 bits is in no way


Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature



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