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

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

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.

-boris