Re: [Xen-devel] [PATCH v3 06/24] x86/pv: Implement pv_inject_{event, page_fault, hw_exception}()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.16 at 14:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> To help with event injection improvements for the PV uses of x86_emulate(),
> implement a event injection API which matches its hvm counterpart.
> 
> This is started with taking do_guest_trap() and modifying its calling API to
> pv_inject_event(), subsequentally implementing the former in terms of the
> latter.
> 
> The existing propagate_page_fault() is fairly similar to
> pv_inject_page_fault(), although it has a return value.  Only a single caller
> makes use of the return value, and non-NULL is only returned if the passed cr2
> is non-canonical.  Opencode this single case in
> handle_gdt_ldt_mapping_fault(), allowing propagate_page_fault() to become
> void.
> 
> The call to reserved_bit_page_fault() in propagate_page_fault() was
> conceptually wrong to start with.  Complaining about reserved bits should be
> part of handling the pagefault itself, not part of injecting a pagefault into
> the guest.  It is therefore moved ahead of the injection call in
> do_page_fault() to compensate.
> 
> The remaining #PF specific bits are moved into pv_inject_event(), and
> pv_inject_page_fault() is implemented as a static inline wrapper.
> 
> No practical change from a guests point of view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Acked-by: Tim Deegan <tim@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -625,37 +625,75 @@ void fatal_trap(const struct cpu_user_regs *regs, 
> bool_t show_remote)
>            (regs->eflags & X86_EFLAGS_IF) ? "" : ", IN INTERRUPT CONTEXT");
>  }
>  
> -static void do_guest_trap(unsigned int trapnr,
> -                          const struct cpu_user_regs *regs)
> +void pv_inject_event(const struct x86_event *event)
>  {
>      struct vcpu *v = current;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>      struct trap_bounce *tb;
>      const struct trap_info *ti;
> +    const uint8_t vector = event->vector;
>      const bool use_error_code =
> -        ((trapnr < 32) && (TRAP_HAVE_EC & (1u << trapnr)));
> +        ((vector < 32) && (TRAP_HAVE_EC & (1u << vector)));
> +    unsigned int error_code = event->error_code;
>  
> -    trace_pv_trap(trapnr, regs->eip, use_error_code, regs->error_code);
> +    ASSERT(vector == event->vector); /* Confirm no truncation. */
> +    if ( use_error_code )
> +        ASSERT(error_code != X86_EVENT_NO_EC);
> +    else
> +        ASSERT(error_code == X86_EVENT_NO_EC);
>  
>      tb = &v->arch.pv_vcpu.trap_bounce;
> -    ti = &v->arch.pv_vcpu.trap_ctxt[trapnr];
> +    ti = &v->arch.pv_vcpu.trap_ctxt[vector];
>  
>      tb->flags = TBF_EXCEPTION;
>      tb->cs    = ti->cs;
>      tb->eip   = ti->address;
>  
> +    if ( vector == TRAP_page_fault )
> +    {
> +        v->arch.pv_vcpu.ctrlreg[2] = event->cr2;
> +        arch_set_cr2(v, event->cr2);
> +
> +        /* Re-set error_code.user flag appropriately for the guest. */
> +        error_code &= ~PFEC_user_mode;
> +        if ( !guest_kernel_mode(v, regs) )
> +            error_code |= PFEC_user_mode;
> +
> +        trace_pv_page_fault(event->cr2, error_code);
> +    }
> +    else
> +        trace_pv_trap(vector, regs->eip, use_error_code, error_code);
> +
>      if ( use_error_code )
>      {
>          tb->flags |= TBF_EXCEPTION_ERRCODE;
> -        tb->error_code = regs->error_code;
> +        tb->error_code = error_code;
>      }
>  
>      if ( TI_GET_IF(ti) )
>          tb->flags |= TBF_INTERRUPT;
>  
>      if ( unlikely(null_trap_bounce(v, tb)) )
> +    {
>          gprintk(XENLOG_WARNING,
>                  "Unhandled %s fault/trap [#%d, ec=%04x]\n",
> -                trapstr(trapnr), trapnr, regs->error_code);
> +                trapstr(vector), vector, error_code);
> +
> +        if ( vector == TRAP_page_fault )
> +            show_page_walk(event->cr2);

Might be worth adding "&& !(regs->error_code & PFEC_reserved_bit)"
to the condition to avoid doing the page walk a second time, perhaps
with a brief comment explaining this.

Jan

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