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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Tue, Sep 6, 2016 at 6:49 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 06/09/16 18:44, George Dunlap wrote:
>
> 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?)
>
> From the docs:
>
> Because there are many reasons why a case intentionally does not end with a
> break statement, the MISSING_BREAK checker does not report defects in cases
> that:
>
> Are followed by a case that starts with a break.
>
> End with a comment. The checker assumes that this comment is acknowledging a
> fallthrough. The comment can start anywhere on the last line, or be a
> multi-line C comment.
>
> Are empty.
>
> Have no control flow path because, for example, there is a return statement.
>
> Have at least one conditional statement that contains a break statement.
>
> Start and end on the same line.
>
> Have a top-level statement that is a call to a function that can end the
> program.
>
> Fall through to another case that has a similar numeric value when
> interpreted as ASCII. Values are considered similar when both are whitespace
> values (such as space, tab, or newline), or the two values are different
> cases (uppercase or lowercase) of the same letter.
>
>
> I suspect we are hitting the "start and end on the same line" exclusion.  It
> is worth noting that the HVM code is already like this, and passing fine.

Cool, thanks.

 -George

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