diff mbox series

[v4,10/11] xen/arm: call hypercall handlers via generated macro

Message ID 20220310073420.15622-11-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen: drop hypercall function tables | expand

Commit Message

Jürgen Groß March 10, 2022, 7:34 a.m. UTC
Instead of using a function table use the generated macros for calling
the appropriate hypercall handlers.

This makes the calls of the handlers type safe.

For deprecated hypercalls define stub functions.

Signed-off-by: Juergen Gross <jgross@suse.com>
Tested-by: Michal Orzel <michal.orzel@arm.com>
---
V2:
- make hypercall_args[] const (Jan Beulich)
---
 xen/arch/arm/traps.c | 124 +++++++++----------------------------------
 1 file changed, 26 insertions(+), 98 deletions(-)

Comments

Julien Grall March 23, 2022, 9:40 a.m. UTC | #1
Hi,

On 10/03/2022 07:34, Juergen Gross wrote:
> @@ -1520,7 +1460,10 @@ static bool check_multicall_32bit_clean(struct multicall_entry *multi)
>   {
>       int i;
>   
> -    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
> +    if ( multi->op >= ARRAY_SIZE(hypercall_args) )
> +        return true;

NIT: This change reads odd to me. So I would prefer...

> +
> +    for ( i = 0; i < hypercall_args[multi->op]; i++ )
>       {
>           if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
>           {
> @@ -1537,28 +1480,13 @@ static bool check_multicall_32bit_clean(struct multicall_entry *multi)
>   enum mc_disposition arch_do_multicall_call(struct mc_state *state)
>   {
>       struct multicall_entry *multi = &state->call;
> -    arm_hypercall_fn_t call = NULL;
> -
> -    if ( multi->op >= ARRAY_SIZE(arm_hypercall_table) )

... if we keep this checks. So we don't return true in 
check_multicall_32bit_clean() when the hypercall doesn't exist.

The code still do the right thing, so either way:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,
Jürgen Groß March 24, 2022, 6:58 a.m. UTC | #2
On 23.03.22 10:40, Julien Grall wrote:
> Hi,
> 
> On 10/03/2022 07:34, Juergen Gross wrote:
>> @@ -1520,7 +1460,10 @@ static bool check_multicall_32bit_clean(struct 
>> multicall_entry *multi)
>>   {
>>       int i;
>> -    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
>> +    if ( multi->op >= ARRAY_SIZE(hypercall_args) )
>> +        return true;
> 
> NIT: This change reads odd to me. So I would prefer...
> 
>> +
>> +    for ( i = 0; i < hypercall_args[multi->op]; i++ )
>>       {
>>           if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
>>           {
>> @@ -1537,28 +1480,13 @@ static bool check_multicall_32bit_clean(struct 
>> multicall_entry *multi)
>>   enum mc_disposition arch_do_multicall_call(struct mc_state *state)
>>   {
>>       struct multicall_entry *multi = &state->call;
>> -    arm_hypercall_fn_t call = NULL;
>> -
>> -    if ( multi->op >= ARRAY_SIZE(arm_hypercall_table) )
> 
> ... if we keep this checks. So we don't return true in 
> check_multicall_32bit_clean() when the hypercall doesn't exist.

The idea was to spare the not necessary check in case of a 64-bit guest.

If you prefer to keep the check here, I'm fine to do it this way.

> 
> The code still do the right thing, so either way:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks,


Juergen
diff mbox series

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index deb07784d9..98eab9a379 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1331,67 +1331,20 @@  static register_t do_deprecated_hypercall(void)
     return -ENOSYS;
 }
 
-typedef register_t (*arm_hypercall_fn_t)(
-    register_t, register_t, register_t, register_t, register_t);
-
-typedef struct {
-    arm_hypercall_fn_t fn;
-    int nr_args;
-} arm_hypercall_t;
-
-#define HYPERCALL(_name, _nr_args)                                   \
-    [ __HYPERVISOR_ ## _name ] =  {                                  \
-        .fn = (arm_hypercall_fn_t) &do_ ## _name,                    \
-        .nr_args = _nr_args,                                         \
-    }
+long dep_sched_op_compat(int cmd, unsigned long arg)
+{
+    return do_deprecated_hypercall();
+}
 
-#define HYPERCALL_ARM(_name, _nr_args)                        \
-    [ __HYPERVISOR_ ## _name ] =  {                                  \
-        .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
-        .nr_args = _nr_args,                                         \
-    }
-/*
- * Only use this for hypercalls which were deprecated (i.e. replaced
- * by something else) before Xen on ARM was created, i.e. *not* for
- * hypercalls which are simply not yet used on ARM.
- */
-#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
-    [ __HYPERVISOR_##_name ] = {                                \
-        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \
-        .nr_args = _nr_args,                                    \
-    }
+long dep_event_channel_op_compat(XEN_GUEST_HANDLE_PARAM(evtchn_op_t) uop)
+{
+    return do_deprecated_hypercall();
+}
 
-static arm_hypercall_t arm_hypercall_table[] = {
-    HYPERCALL(memory_op, 2),
-    HYPERCALL(domctl, 1),
-    HYPERCALL(sched_op, 2),
-    HYPERCALL_DEPRECATED(sched_op_compat, 2),
-    HYPERCALL(console_io, 3),
-    HYPERCALL(xen_version, 2),
-    HYPERCALL(xsm_op, 1),
-    HYPERCALL(event_channel_op, 2),
-    HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
-    HYPERCALL_ARM(physdev_op, 2),
-    HYPERCALL_DEPRECATED(physdev_op_compat, 1),
-    HYPERCALL(sysctl, 2),
-    HYPERCALL(hvm_op, 2),
-#ifdef CONFIG_GRANT_TABLE
-    HYPERCALL(grant_table_op, 3),
-#endif
-    HYPERCALL(multicall, 2),
-    HYPERCALL(platform_op, 1),
-    HYPERCALL(vcpu_op, 3),
-    HYPERCALL(vm_assist, 2),
-#ifdef CONFIG_ARGO
-    HYPERCALL(argo_op, 5),
-#endif
-#ifdef CONFIG_HYPFS
-    HYPERCALL(hypfs_op, 5),
-#endif
-#ifdef CONFIG_IOREQ_SERVER
-    HYPERCALL(dm_op, 3),
-#endif
-};
+long dep_physdev_op_compat(XEN_GUEST_HANDLE_PARAM(physdev_op_t) uop)
+{
+    return do_deprecated_hypercall();
+}
 
 #ifndef NDEBUG
 static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
@@ -1430,7 +1383,6 @@  static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 #define HYPERCALL_ARG3(r) (r)->x2
 #define HYPERCALL_ARG4(r) (r)->x3
 #define HYPERCALL_ARG5(r) (r)->x4
-#define HYPERCALL_ARGS(r) (r)->x0, (r)->x1, (r)->x2, (r)->x3, (r)->x4
 #else
 #define HYPERCALL_RESULT_REG(r) (r)->r0
 #define HYPERCALL_ARG1(r) (r)->r0
@@ -1438,52 +1390,40 @@  static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 #define HYPERCALL_ARG3(r) (r)->r2
 #define HYPERCALL_ARG4(r) (r)->r3
 #define HYPERCALL_ARG5(r) (r)->r4
-#define HYPERCALL_ARGS(r) (r)->r0, (r)->r1, (r)->r2, (r)->r3, (r)->r4
 #endif
 
+static const unsigned char hypercall_args[] = hypercall_args_arm;
+
 static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
                               const union hsr hsr)
 {
-    arm_hypercall_fn_t call = NULL;
     struct vcpu *curr = current;
 
-    BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
-
     if ( hsr.iss != XEN_HYPERCALL_TAG )
     {
         gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
         return inject_undef_exception(regs, hsr);
     }
 
-    if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
-    {
-        perfc_incr(invalid_hypercalls);
-        HYPERCALL_RESULT_REG(regs) = -ENOSYS;
-        return;
-    }
-
     curr->hcall_preempted = false;
 
     perfc_incra(hypercalls, *nr);
-    call = arm_hypercall_table[*nr].fn;
-    if ( call == NULL )
-    {
-        HYPERCALL_RESULT_REG(regs) = -ENOSYS;
-        return;
-    }
 
-    HYPERCALL_RESULT_REG(regs) = call(HYPERCALL_ARGS(regs));
+    call_handlers_arm(*nr, HYPERCALL_RESULT_REG(regs), HYPERCALL_ARG1(regs),
+                      HYPERCALL_ARG2(regs), HYPERCALL_ARG3(regs),
+                      HYPERCALL_ARG4(regs), HYPERCALL_ARG5(regs));
 
 #ifndef NDEBUG
-    if ( !curr->hcall_preempted )
+    if ( !curr->hcall_preempted && HYPERCALL_RESULT_REG(regs) != -ENOSYS )
     {
         /* Deliberately corrupt parameter regs used by this hypercall. */
-        switch ( arm_hypercall_table[*nr].nr_args ) {
+        switch ( hypercall_args[*nr] ) {
         case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEF;
         case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEF;
         case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEF;
         case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEF;
         case 1: /* Don't clobber x0/r0 -- it's the return value */
+        case 0: /* -ENOSYS case */
             break;
         default: BUG();
         }
@@ -1520,7 +1460,10 @@  static bool check_multicall_32bit_clean(struct multicall_entry *multi)
 {
     int i;
 
-    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
+    if ( multi->op >= ARRAY_SIZE(hypercall_args) )
+        return true;
+
+    for ( i = 0; i < hypercall_args[multi->op]; i++ )
     {
         if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
         {
@@ -1537,28 +1480,13 @@  static bool check_multicall_32bit_clean(struct multicall_entry *multi)
 enum mc_disposition arch_do_multicall_call(struct mc_state *state)
 {
     struct multicall_entry *multi = &state->call;
-    arm_hypercall_fn_t call = NULL;
-
-    if ( multi->op >= ARRAY_SIZE(arm_hypercall_table) )
-    {
-        multi->result = -ENOSYS;
-        return mc_continue;
-    }
-
-    call = arm_hypercall_table[multi->op].fn;
-    if ( call == NULL )
-    {
-        multi->result = -ENOSYS;
-        return mc_continue;
-    }
 
     if ( is_32bit_domain(current->domain) &&
          !check_multicall_32bit_clean(multi) )
         return mc_continue;
 
-    multi->result = call(multi->args[0], multi->args[1],
-                         multi->args[2], multi->args[3],
-                         multi->args[4]);
+    call_handlers_arm(multi->op, multi->result, multi->args[0], multi->args[1],
+                      multi->args[2], multi->args[3], multi->args[4]);
 
     return likely(!regs_mode_is_user(guest_cpu_user_regs()))
            ? mc_continue : mc_preempt;