From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection Date: Fri, 26 Sep 2014 16:13:14 -0400 Message-ID: <5425C8DA.3000904@oracle.com> References: <1411484611-31027-1-git-send-email-andrew.cooper3@citrix.com> <1411484611-31027-5-git-send-email-andrew.cooper3@citrix.com> <5422C0AE.3090404@oracle.com> <5422C15D.7030003@citrix.com> <5422C60E.6040708@oracle.com> <5422D327.80804@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5422D327.80804@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Xen-devel Cc: Aravind Gopalakrishnan , Suravee Suthikulpanit , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 >>>>> Signed-off-by: Andrew Cooper >>>>> Signed-off-by: Jan Beulich >>>>> CC: Boris Ostrovsky >>>>> CC: Suravee Suthikulpanit >>>>> CC: Aravind Gopalakrishnan >>>>> --- >>>>> 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> 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