Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/24/2014 10:20 AM, Andrew Cooper wrote:
> On 24/09/14 14:24, Boris Ostrovsky wrote:
>> On 09/24/2014 09:04 AM, Andrew Cooper wrote:
>>> On 24/09/14 14:01, Boris Ostrovsky wrote:
>>>> On 09/23/2014 11:03 AM, Andrew Cooper wrote:
>>>>> AMD SVM requires all software events to have their injection
>>>>> emulated if
>>>>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>>>>> injection
>>>>> requires emulation in all cases, even with hardware NextRIP support.
>>>>>
>>>>> Emulating full control transfers is overkill for our needs.  All that
>>>>> matters
>>>>> is that guest userspace can't bypass the descriptor DPL check.  Any
>>>>> guest OS
>>>>> which would incur other faults as part of injection is going to end
>>>>> up with a
>>>>> double fault instead, and won't be in a position to care that the
>>>>> faulting eip
>>>>> is wrong.
>>>>>
>>>>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>>> ---
>>>>>     xen/arch/x86/hvm/emulate.c             |    8 +++
>>>>>     xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>>>>     xen/arch/x86/mm.c                      |    2 +
>>>>>     xen/arch/x86/mm/shadow/common.c        |    1 +
>>>>>     xen/arch/x86/x86_emulate/x86_emulate.c |  122
>>>>> ++++++++++++++++++++++++++++++--
>>>>>     xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>>>>     6 files changed, 191 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>>> index 7ee146b..463ccfb 100644
>>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>>> @@ -21,6 +21,7 @@
>>>>>     #include <asm/hvm/hvm.h>
>>>>>     #include <asm/hvm/trace.h>
>>>>>     #include <asm/hvm/support.h>
>>>>> +#include <asm/hvm/svm/svm.h>
>>>>>       static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>>>>     {
>>>>> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct
>>>>> hvm_emulate_ctxt *hvmemul_ctxt,
>>>>>         vio->mmio_retrying = vio->mmio_retry;
>>>>>         vio->mmio_retry = 0;
>>>>>     +    if ( cpu_has_vmx )
>>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>>>>> +    else if ( cpu_has_svm_nrips )
>>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
>>>>> +    else
>>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>>>>> +
>>>>>         rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>>>>>           if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>>> index de982fd..b6beefc 100644
>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>>>>> *trap)
>>>>>         struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>>>>         eventinj_t event = vmcb->eventinj;
>>>>>         struct hvm_trap _trap = *trap;
>>>>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>>>           switch ( _trap.vector )
>>>>>         {
>>>>>         case TRAP_debug:
>>>>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>>>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>>>>             {
>>>>>                 __restore_debug_registers(vmcb, curr);
>>>>>                 vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>>>>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>>>>> *trap)
>>>>>           event.bytes = 0;
>>>>>         event.fields.v = 1;
>>>>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>>>         event.fields.vector = _trap.vector;
>>>>> -    event.fields.ev = (_trap.error_code !=
>>>>> HVM_DELIVER_NO_ERROR_CODE);
>>>>> -    event.fields.errorcode = _trap.error_code;
>>>>> +
>>>>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>>>>> Injection. */
>>>>> +    switch ( _trap.type )
>>>>> +    {
>>>>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>>>>> +        /*
>>>>> +         * Injection type 4 (software interrupt) is only supported
>>>>> with
>>>>> +         * NextRIP support.  Without NextRIP, the emulator will have
>>>>> performed
>>>>> +         * DPL and presence checks for us.
>>>>> +         */
>>>>> +        if ( cpu_has_svm_nrips )
>>>>> +        {
>>>>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>>>>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>>>>> +        }
>>>>> +        else
>>>>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>>> +        break;
>>>>> +
>>>>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>>>>> +        /*
>>>>> +         * icebp's injection must always be emulated.  Software
>>>>> injection help
>>>>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>>>>> used) still
>>>>> +         * needs setting or execution will resume from 0.
>>>>> +         */
>>>> Can you tell me where eip is updated? I don't see any difference
>>>> between how, for example, int3 is emulated differently from icebp.
>>>>
>>>> -boris
>>> The second hunk in this series, chooses between x86_swint_emulate_icebp
>>> and x86_swint_emulate_all based on NRIP support.
>>>
>>> This cause inject_swint() to either emulate the injection or not.
>> I was asking about why you are calculating nextrip differently (i.e
>> not adding _trap.insn_len) for icebp vs. int3. I read the comment as
>> if it was saying that x86_emulate() has already taken care of that.
>> And I don't understand where that is done.
> With NRIPS support, hardware can deal with injecting the trap and doing
> DPL/presence checks, which means the eip in the exception frame might
> either be a trap or a fault.
>
> regs->eip and vmcb->nextrip bound the trapping instruction, with eip
> pointing at it, and nextrip pointing after it.  These are the two
> choices which get set in the exception frame, after hardware has decided
> whether to send the trap, or fault because of a bad IDT setup.
>
> Without NRIP support, the emulator must move regs->eip on to get the
> trap frame eip pointing at the correct address as it is the only
> available choice for an exception frame.
>
> For ICEBP, there is no hardware support for injection at all (even with
> NRIP available).  The emulator must move regs->eip forward to get the
> correct trap eip for the non-NRIP case, but with NRIP support,
> vmcb->nextrip must be set, or the resulting trap frame contains an eip of 0.
>
> I suppose this looks a little weird because of the intersection of
> x86_swint_emulate_icebp and x86_swint_emulate_all, but it is not correct
> to inject a software event with NRIP support for icebp, as that would
> cause hardware to do a DPL check and fail to set the external bit in a
> #NP error code.

Thanks.

My problem was that I didn't understand what the first 'if' in 
inject_swint() was doing. I first read it as if it was trying to figure 
out which instruction is being emulated.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>