Re: [Xen-devel] [V10 PATCH 11/23] PVH xen: support invalid op emulation for PVH

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> This patch supports invalid op emulation for PVH by calling appropriate
> copy macros and and HVM function to inject PF.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/arch/x86/traps.c        |   17 ++++++++++++++---
>  xen/include/asm-x86/traps.h |    1 +
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 378ef0a..a3ca70b 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -459,6 +459,11 @@ static void instruction_done(
>      struct cpu_user_regs *regs, unsigned long eip, unsigned int bpmatch)
>  {
>      regs->eip = eip;
> +
> +    /* PVH fixme: debug trap below */
> +    if ( is_pvh_vcpu(current) )
> +        return;

What exactly does this comment mean?  Do you mean, "FIXME: Make debug
trapping work for PVH guests"?  (i.e., this functionality will be
implemented later?)

> +
>      regs->eflags &= ~X86_EFLAGS_RF;
>      if ( bpmatch || (regs->eflags & X86_EFLAGS_TF) )
>      {
> @@ -913,7 +918,7 @@ static int emulate_invalid_rdtscp(struct cpu_user_regs 
> *regs)
>      return EXCRET_fault_fixed;
>  }
>
> -static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
> +int emulate_forced_invalid_op(struct cpu_user_regs *regs)

Why make this non-static?  No one is using this in this patch.  If a
later patch needs it, you should make it non-static there, so we can
decide at that point if making it non-static is merited or not.

>  {
>      char sig[5], instr[2];
>      unsigned long eip, rc;
> @@ -921,7 +926,7 @@ static int emulate_forced_invalid_op(struct cpu_user_regs 
> *regs)
>      eip = regs->eip;
>
>      /* Check for forced emulation signature: ud2 ; .ascii "xen". */
> -    if ( (rc = copy_from_user(sig, (char *)eip, sizeof(sig))) != 0 )
> +    if ( (rc = raw_copy_from_guest(sig, (char *)eip, sizeof(sig))) != 0 )
>      {
>          propagate_page_fault(eip + sizeof(sig) - rc, 0);
>          return EXCRET_fault_fixed;
> @@ -931,7 +936,7 @@ static int emulate_forced_invalid_op(struct cpu_user_regs 
> *regs)
>      eip += sizeof(sig);
>
>      /* We only emulate CPUID. */
> -    if ( ( rc = copy_from_user(instr, (char *)eip, sizeof(instr))) != 0 )
> +    if ( ( rc = raw_copy_from_guest(instr, (char *)eip, sizeof(instr))) != 0 
> )
>      {
>          propagate_page_fault(eip + sizeof(instr) - rc, 0);
>          return EXCRET_fault_fixed;
> @@ -1076,6 +1081,12 @@ void propagate_page_fault(unsigned long addr, u16 
> error_code)
>      struct vcpu *v = current;
>      struct trap_bounce *tb = &v->arch.pv_vcpu.trap_bounce;
>
> +    if ( is_pvh_vcpu(v) )
> +    {
> +        hvm_inject_page_fault(error_code, addr);
> +        return;
> +    }

Would it make more sense to rename this function
"pv_inject_page_fault", and then make a macro to switch between the
two?


> +
>      v->arch.pv_vcpu.ctrlreg[2] = addr;
>      arch_set_cr2(v, addr);
>
> diff --git a/xen/include/asm-x86/traps.h b/xen/include/asm-x86/traps.h
> index 82cbcee..1d9b087 100644
> --- a/xen/include/asm-x86/traps.h
> +++ b/xen/include/asm-x86/traps.h
> @@ -48,5 +48,6 @@ extern int guest_has_trap_callback(struct domain *d, 
> uint16_t vcpuid,
>   */
>  extern int send_guest_trap(struct domain *d, uint16_t vcpuid,
>                                 unsigned int trap_nr);
> +int emulate_forced_invalid_op(struct cpu_user_regs *regs);

Same as above re making the function non-static.

 -George

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