Re: [PATCH 10/12] xen/x86: call hypercall handlers via switch statement

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2021 14:51, Juergen Gross wrote:
> Instead of using a function table use the generated switch statement
> macros for calling the appropriate hypercall handlers.
> 
> This is beneficial to performance and avoids speculation issues.
> 
> With calling the handlers using the correct number of parameters now
> it is possible to do the parameter register clobbering in the NDEBUG
> case after returning from the handler. This in turn removes the only
> users of hypercall_args_table[] which can be removed now.

"removed" reads misleading to me: You really replace it by new tables,
using script-generated initializers. Also it looks like you're doubling
the data, as the same sets were previously used by pv64/hvm64 and
pv32/hvm32 respectively.

> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -108,56 +108,10 @@ long hvm_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          return compat_physdev_op(cmd, arg);
>  }
>  
> -#define HYPERCALL(x)                                         \
> -    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
> -                               (hypercall_fn_t *) do_ ## x }
> -
> -#define HVM_CALL(x)                                          \
> -    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) hvm_ ## x, \
> -                               (hypercall_fn_t *) hvm_ ## x }
> -
> -#define COMPAT_CALL(x)                                       \
> -    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
> -                               (hypercall_fn_t *) compat_ ## x }
> -
> -static const struct {
> -    hypercall_fn_t *native, *compat;
> -} hvm_hypercall_table[] = {
> -    HVM_CALL(memory_op),
> -    COMPAT_CALL(multicall),
> -#ifdef CONFIG_GRANT_TABLE
> -    HVM_CALL(grant_table_op),
> -#endif
> -    HYPERCALL(vm_assist),
> -    COMPAT_CALL(vcpu_op),
> -    HVM_CALL(physdev_op),
> -    COMPAT_CALL(xen_version),
> -    HYPERCALL(console_io),
> -    HYPERCALL(event_channel_op),
> -    COMPAT_CALL(sched_op),
> -    COMPAT_CALL(set_timer_op),
> -    COMPAT_CALL(xsm_op),
> -    HYPERCALL(hvm_op),
> -    HYPERCALL(sysctl),
> -    HYPERCALL(domctl),
> -#ifdef CONFIG_ARGO
> -    COMPAT_CALL(argo_op),
> -#endif
> -    COMPAT_CALL(platform_op),
> -#ifdef CONFIG_PV
> -    COMPAT_CALL(mmuext_op),
> -#endif
> -    HYPERCALL(xenpmu_op),
> -    COMPAT_CALL(dm_op),
> -#ifdef CONFIG_HYPFS
> -    HYPERCALL(hypfs_op),
> +#ifndef NDEBUG
> +static unsigned char hypercall_args_64[] = hypercall_args_hvm64;
> +static unsigned char hypercall_args_32[] = hypercall_args_hvm32;

Irrespective of this being debugging-only: Const?

> @@ -239,33 +176,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>          HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx)",
>                      eax, rdi, rsi, rdx, r10, r8);
>  
> -#ifndef NDEBUG
> -        /* Deliberately corrupt parameter regs not used by this hypercall. */
> -        switch ( hypercall_args_table[eax].native )
> -        {
> -        case 0: rdi = 0xdeadbeefdeadf00dUL; fallthrough;
> -        case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
> -        case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
> -        case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
> -        case 4: r8 = 0xdeadbeefdeadf00dUL;
> -        }
> -#endif
> -
> -        regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
> +        call_handlers_hvm64(eax, regs->rax, rdi, rsi, rdx, r10, r8);
>  
>  #ifndef NDEBUG
> -        if ( !curr->hcall_preempted )
> -        {
> -            /* Deliberately corrupt parameter regs used by this hypercall. */
> -            switch ( hypercall_args_table[eax].native )
> -            {
> -            case 5: regs->r8  = 0xdeadbeefdeadf00dUL; fallthrough;
> -            case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
> -            case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
> -            case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
> -            case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
> -            }
> -        }
> +        if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
> +            clobber_regs(regs, hypercall_args_64[eax]);

I'm not fundamentally opposed, but sadly -ENOSYS comes back also in undue
situations, e.g. various hypercalls still produce this for "unknown
sub-function". Hence the weakened clobbering wants at least mentioning,
perhaps also justifying, in the description.

> @@ -55,4 +42,34 @@ compat_common_vcpu_op(
>  
>  #endif /* CONFIG_COMPAT */
>  
> +#ifndef NDEBUG

Hmm, I was actuall hoping for the conditional to actually live ...

> +static inline void clobber_regs(struct cpu_user_regs *regs,
> +                                unsigned int nargs)
> +{

... here and ...

> +    /* Deliberately corrupt used parameter regs. */
> +    switch ( nargs )
> +    {
> +    case 5: regs->r8  = 0xdeadbeefdeadf00dUL; fallthrough;
> +    case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
> +    case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
> +    case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
> +    case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
> +    }
> +}
> +
> +static inline void clobber_regs32(struct cpu_user_regs *regs,
> +                                  unsigned int nargs)
> +{

... here, such that the conditionals in the .c files could go away
altogether.

> +    /* Deliberately corrupt used parameter regs. */
> +    switch ( nargs )
> +    {
> +    case 5: regs->edi = 0xdeadf00dUL; fallthrough;
> +    case 4: regs->esi = 0xdeadf00dUL; fallthrough;
> +    case 3: regs->edx = 0xdeadf00dUL; fallthrough;
> +    case 2: regs->ecx = 0xdeadf00dUL; fallthrough;
> +    case 1: regs->ebx = 0xdeadf00dUL;

No need for the UL suffixes here afaics; U ones may want to be there.

Overall, besides these mainly cosmetic aspects the main thing missing
is an approach to prioritize the handful most frequently used functions,
for them to be pulled out of the switch() so we don't depend on the
compiler's choice for the order of comparisons done.

Jan