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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

~Andrew