Re: [Xen-devel] [PATCH v2 4/9] x86/pv: Implement pv_hypercall() in C

[George Dunlap <dunlapg@xxxxxxxxx>]

On Tue, Sep 6, 2016 at 11:12 AM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> In a similar style to hvm_do_hypercall().  The C version is far easier to
> understand and edit than the assembly versions.
>
> There are a few small differences however.  The register clobbering values
> have changed (to match the HVM side), and in particular clobber the upper
> 32bits of 64bit arguments.  The hypercall and performance counter record are
> reordered to increase code sharing between the 32bit and 64bit cases.
>
> The sole callers of __trace_hypercall_entry() were the assembly code.  Given
> the new C layout, it is more convenient to fold __trace_hypercall_entry() into
> pv_hypercall(), and call __trace_hypercall() directly.
>
> Finally, pv_hypercall() will treat a NULL hypercall function pointer as
> -ENOSYS, allowing further cleanup.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
>
> v2:
>  * Use guest_mode_kernel() rather than TF_kernel_mode
>  * Consistent use of 32bit stores
>  * Don't truncate rax for 64bit domains
>  * Move eax return assignment into C
> ---
>  xen/arch/x86/hypercall.c           | 120 
> +++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/trace.c               |  27 ---------
>  xen/arch/x86/x86_64/asm-offsets.c  |   1 -
>  xen/arch/x86/x86_64/compat/entry.S |  69 +--------------------
>  xen/arch/x86/x86_64/entry.S        |  61 +------------------
>  5 files changed, 124 insertions(+), 154 deletions(-)
>
> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
> index 4b42f86..13a89a0 100644
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -17,7 +17,12 @@
>   * Copyright (c) 2015,2016 Citrix Systems Ltd.
>   */
>
> +#include <xen/compiler.h>
>  #include <xen/hypercall.h>
> +#include <xen/trace.h>
> +
> +extern hypercall_fn_t *const hypercall_table[NR_hypercalls],
> +    *const compat_hypercall_table[NR_hypercalls];
>
>  #define ARGS(x, n)                              \
>      [ __HYPERVISOR_ ## x ] = (n)
> @@ -111,6 +116,121 @@ const uint8_t 
> compat_hypercall_args_table[NR_hypercalls] =
>
>  #undef ARGS
>
> +void pv_hypercall(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *curr = current;
> +#ifndef NDEBUG
> +    unsigned long old_rip = regs->rip;
> +#endif
> +    unsigned long eax;
> +
> +    ASSERT(guest_kernel_mode(curr, regs));
> +
> +    eax = is_pv_32bit_vcpu(curr) ? regs->_eax : regs->eax;
> +
> +    if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
> +    {
> +        regs->eax = -ENOSYS;
> +        return;
> +    }
> +
> +    if ( !is_pv_32bit_vcpu(curr) )
> +    {
> +        unsigned long rdi = regs->rdi;
> +        unsigned long rsi = regs->rsi;
> +        unsigned long rdx = regs->rdx;
> +        unsigned long r10 = regs->r10;
> +        unsigned long r8 = regs->r8;
> +        unsigned long r9 = regs->r9;
> +
> +#ifndef NDEBUG
> +        /* Deliberately corrupt parameter regs not used by this hypercall. */
> +        switch ( hypercall_args_table[eax] )
> +        {
> +        case 0: rdi = 0xdeadbeefdeadf00dUL;
> +        case 1: rsi = 0xdeadbeefdeadf00dUL;
> +        case 2: rdx = 0xdeadbeefdeadf00dUL;
> +        case 3: r10 = 0xdeadbeefdeadf00dUL;
> +        case 4: r8 = 0xdeadbeefdeadf00dUL;
> +        case 5: r9 = 0xdeadbeefdeadf00dUL;
> +        }

Out of curiosity, is Coverity going to complain about the lack of /*
FALLTHROUGH */ in these stanzas, or is there some magic that lets it
know this is intended?  (Or does it ignore DEBUG code?)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel