Re: [Xen-devel] [PATCH for-4.7] x86/hvm: Correct emulation of invlpg instruction

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 22 April 2016 10:31
> To: Andrew Cooper
> Cc: Paul Durrant; Wei Liu; Xen-devel
> Subject: Re: [PATCH for-4.7] x86/hvm: Correct emulation of invlpg instruction
> 
> >>> On 22.04.16 at 10:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> > `invlpg` and `invlpga` are specified to be NOPs when issued on non-
> canonical
> > addresses.
> >
> > These instructions are not normally intercepted.  They are however
> > intercepted
> > for HVM guests running in shadow paging mode.  AMD hardware lacking
> decode
> > hardware assistance uses the general instruction emulator to handle the
> > interception.
> >
> > Alter hvmemul_invlpg() to swallow the #GP exception resulting from a
> > non-canonical address, rather than reporting it back to the guest.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -1598,8 +1598,27 @@ static int hvmemul_invlpg(
> >      rc = hvmemul_virtual_to_linear(
> >          seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
> >
> > -    if ( rc == X86EMUL_OKAY )
> > +    switch ( rc )
> > +    {
> > +    case X86EMUL_OKAY:
> >          hvm_funcs.invlpg_intercept(addr);
> > +        break;
> > +
> > +    case X86EMUL_EXCEPTION:
> > +        ASSERT(hvmemul_ctxt->trap.vector == TRAP_gp_fault);
> > +        /*
> > +         * `invlpg` and `invlpga` are specified to be NOPs when issued on a
> > +         * non-canonical address.  hvmemul_virtual_to_linear() latches a 
> > #GP
> > +         * which is the useful behaviour for most of its callers.
> 
> Here and in the description I'd prefer you to not exclusively refer
> to non-canonical addresses - segment limit violations in 32-bit or
> compatibility modes are affected as much.

...in which case squashing the #GP would be incorrect, right?

> 
> > +         * Clear the pending exception to match avoid delivering a #GP 
> > fault
> > +         * to the guest.
> > +         */
> > +        hvmemul_ctxt->exn_pending = 0;
> > +        hvmemul_ctxt->trap = (struct hvm_trap){};
> 
> memset() would in this case look more natural I think, but is this
> field really meaningful in the first place for exn_pending cleared?

Still probably good practise to clear the trap, but memset() would indeed be 
more readable.

  Paul

> 
> Jan


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