Re: [Xen-devel] [V0 PATCH 3/6] AMD-PVH: call hvm_emulate_one instead of handle_mmio

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Fri, 22 Aug 2014 10:50:01 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 16.08.14 at 03:53, <mukesh.rathor@xxxxxxxxxx> wrote:
> > Certain IOIO instructions and CR access instructions like
> > lmsw/clts etc need to be emulated. handle_mmio is incorrectly
> > called to accomplish this. Create svm_emulate() to call
> > hvm_emulate_one which is more appropriate, and works for pvh as
> > well.
> 
> Is that really correct without slight adjustments to the function? I'm
> particularly worried about (some of) the uses of vio there...
> 
> > handle_mmio call is forbidden for pvh.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/svm/svm.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index 4ff4a96..dac16f4 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -2209,6 +2209,18 @@ static struct hvm_function_table __initdata
> > svm_function_table = { .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m,
> >  };
> >  
> > +static void svm_emulate(struct cpu_user_regs *regs)
> > +{
> > +    int rc;
> > +    struct hvm_emulate_ctxt ctxt;
> > +
> > +    hvm_emulate_prepare(&ctxt, regs);
> > +    rc = hvm_emulate_one(&ctxt);
> > +
> > +    if ( rc != X86EMUL_OKAY )
> > +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
> 
> I think this is too simplistic - when ctxt.exn_pending is set, you
> ought to be delivering _that_ exception instead of #GP.

Right, the existing code just injects GP, which is not quite ideal. 

> > @@ -2470,16 +2482,16 @@ void svm_vmexit_handler(struct
> > cpu_user_regs *regs) if ( handle_pio(port, bytes, dir) )
> >                  __update_guest_eip(regs, vmcb->exitinfo2 -
> > vmcb->rip); }
> > -        else if ( !handle_mmio() )
> > -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > +        else
> > +            svm_emulate(regs);
> >          break;
> >  
> >      case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
> >      case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
> >          if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL <<
> > 63)) ) svm_vmexit_do_cr_access(vmcb, regs);
> > -        else if ( !handle_mmio() ) 
> > -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > +        else
> > +            svm_emulate(regs);
> >          break;
> >  
> >      case VMEXIT_INVLPG:
> 
> There's another use of handle_mmio() right below here - shouldn't
> that get replaced too (even if not strictly required by PVH)?
> 
> Also - how come at least the use of the function in VMX's
> EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
> but SVM's VMEXIT_IOIO one is?

Yup, missed that one. That would need to be addressed. 

I guess the first step would be to do a non-pvh patch to fix calling
handle_mmio for non-mmio purposes.  Since, it applies to both vmx/svm, 
perhaps an hvm function. Let me do that first, and then pvh can
piggyback on that.

thanks
Mukesh


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