Re: [Xen-devel] [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.17 at 17:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> +void pv_create_exception_frame(void)
> +{
> +    struct vcpu *curr = current;
> +    struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;

const (twice)?

> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const bool user_mode_frame = !guest_kernel_mode(curr, regs);
> +    uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
> +    unsigned long rflags;

Does this really need to be "long"?

> +    unsigned int bytes, missing;
> +
> +    ASSERT_NOT_IN_ATOMIC();
> +
> +    if ( unlikely(null_trap_bounce(curr, tb)) )
> +    {
> +        gprintk(XENLOG_ERR, "Fatal: Attempting to inject null trap 
> bounce\n");
> +        __domain_crash_synchronous();

Why not domain_crash() followed by "return"?

> +    }
> +
> +    /* Fold the upcall mask and architectural IOPL into the guests rflags. */
> +    rflags  = regs->rflags & ~(X86_EFLAGS_IF | X86_EFLAGS_IOPL);
> +    rflags |= ((*evt_mask ? 0 : X86_EFLAGS_IF) |
> +               (VM_ASSIST(curr->domain, architectural_iopl)
> +                ? curr->arch.pv_vcpu.iopl : 0));
> +
> +    if ( is_pv_32bit_vcpu(curr) )
> +    {
> +        /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
> +        unsigned int frame[6], *ptr = frame, ksp =
> +            (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
> +
> +        if ( tb->flags & TBF_EXCEPTION_ERRCODE )
> +            *ptr++ = tb->error_code;
> +
> +        *ptr++ = regs->eip;
> +        *ptr++ = regs->cs | (((unsigned int)*evt_mask) << 16);

Do you really need the cast here? In no case is there a need for the
parentheses around the cast expression.

> +        *ptr++ = rflags;
> +
> +        if ( user_mode_frame )
> +        {
> +            *ptr++ = regs->esp;
> +            *ptr++ = regs->ss;
> +        }
> +
> +        /* Copy the constructed frame to the guest kernel stack. */
> +        bytes = _p(ptr) - _p(frame);
> +        ksp -= bytes;
> +
> +        if ( unlikely((missing = __copy_to_user(_p(ksp), frame, bytes)) != 
> 0) )

While I don't think we need to be really bothered, it's perhaps still
worth noting in a comment that the wrapping behavior here is
wrong (and slightly worse than the assembly original), due to
(implicit) address arithmetic all being done with 64-bit operands.

> +        {
> +            gprintk(XENLOG_ERR, "Fatal: Fault while writing exception 
> frame\n");
> +            show_page_walk(ksp + missing);
> +            __domain_crash_synchronous();
> +        }
> +
> +        /* Rewrite our stack frame. */
> +        regs->rip           = (uint32_t)tb->eip;
> +        regs->cs            = tb->cs;
> +        regs->eflags       &= ~(X86_EFLAGS_VM | X86_EFLAGS_RF |
> +                                X86_EFLAGS_NT | X86_EFLAGS_TF);

You write ->rip above and ->rsp below - preferably those would
become ->eip and ->esp, but alternatively (for consistency) this
may want switching to ->rflags.

> +        regs->rsp           = ksp;
> +        if ( user_mode_frame )
> +            regs->ss = curr->arch.pv_vcpu.kernel_ss;
> +    }
> +    else
> +    {
> +        /* { RCX, R11, [ERRCODE,] RIP, CS/MASK, RFLAGS, RSP, SS } */
> +        unsigned long frame[7], *ptr = frame, ksp =

I clearly count 8 elements in the comment.

> +            (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->rsp) & 
> ~0xf;
> +
> +        if ( user_mode_frame )
> +            toggle_guest_mode(curr);
> +
> +        *ptr++ = regs->rcx;
> +        *ptr++ = regs->r11;
> +
> +        if ( tb->flags & TBF_EXCEPTION_ERRCODE )
> +            *ptr++ = tb->error_code;
> +
> +        *ptr++ = regs->rip;
> +        *ptr++ = (user_mode_frame ? regs->cs : regs->cs & ~3) |
> +            ((unsigned long)(*evt_mask) << 32);

Stray parentheses again.

> +        *ptr++ = rflags;
> +        *ptr++ = regs->rsp;
> +        *ptr++ = regs->ss;
> +
> +        /* Copy the constructed frame to the guest kernel stack. */
> +        bytes = _p(ptr) - _p(frame);
> +        ksp -= bytes;
> +
> +        if ( unlikely(!__addr_ok(ksp)) )
> +        {
> +            gprintk(XENLOG_ERR, "Fatal: Bad guest kernel stack %p\n", 
> _p(ksp));
> +            __domain_crash_synchronous();
> +        }
> +        else if ( unlikely((missing =
> +                            __copy_to_user(_p(ksp), frame, bytes)) != 0) )
> +        {
> +            gprintk(XENLOG_ERR, "Fatal: Fault while writing exception 
> frame\n");
> +            show_page_walk(ksp + missing);
> +            __domain_crash_synchronous();
> +        }
> +
> +        /* Rewrite our stack frame. */
> +        regs->entry_vector |= TRAP_syscall;
> +        regs->rip           = tb->eip;
> +        regs->cs            = FLAT_KERNEL_CS;
> +        regs->rflags       &= ~(X86_EFLAGS_AC | X86_EFLAGS_VM | 
> X86_EFLAGS_RF |
> +                                X86_EFLAGS_NT | X86_EFLAGS_TF);
> +        regs->rsp           = ksp;
> +        regs->ss            = FLAT_KERNEL_SS;
> +    }
> +
> +    /* Mask events if requested. */
> +    if ( tb->flags & TBF_INTERRUPT )
> +        *evt_mask = 1;
> +
> +    /*
> +     * Clobber the injection information now it has been completed.  Buggy
> +     * attempts to inject the same event twice will hit the 
> null_trap_bounce()
> +     * check above.
> +     */
> +    *tb = (struct trap_bounce){};

Ah, so that prevents tb becoming a pointer to const. I wonder
though whether, on a rather hot path, we really want to zap the
entire structure here. As I can see the value in satisfying
null_trap_bounce(), how about zapping just ->eip / ->cs on the
split paths above?

Overall, did you compare generated code with the current
assembly implementation? That one surely would have had some
room for improvement, so the result here at least shouldn't be
worse than that.

Jan

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