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: Wed, 24 Sep 2014 09:24:30 -0400 Message-ID: <5422C60E.6040708@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5422C15D.7030003@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 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. -boris