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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Wed, 7 Aug 2013 12:29:13 +0100
George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:

> 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?)

Correct, future work. Look at what the db trap is doing and make
it work for PVH if it doesn't already.

> > +
> >      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.

Sigh! Originally, it was that way, but then to keep that patch from
getting too big, it got moved here after few versions. We are making
emulation available for outside the PV, ie, to PVH.

> > +    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?

I don't think so, propagate_page_fault seems generic enough.


-mukesh


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