From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V11 PATCH 20/21] PVH xen: introduce vmexit handler for PVH Date: Fri, 23 Aug 2013 17:35:27 -0700 Message-ID: <20130823173527.231e7ff0@mantra.us.oracle.com> References: <1377220750-19514-1-git-send-email-mukesh.rathor@oracle.com> <1377220750-19514-21-git-send-email-mukesh.rathor@oracle.com> <5217439002000078000EDF4C@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VD1pF-0007jo-Og for xen-devel@lists.xenproject.org; Sat, 24 Aug 2013 00:35:33 +0000 In-Reply-To: <5217439002000078000EDF4C@nat28.tlf.novell.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: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Fri, 23 Aug 2013 10:12:16 +0100 "Jan Beulich" wrote: > >>> On 23.08.13 at 03:19, Mukesh Rathor > >>> wrote: > > Changes in V11: > > - merge this with previous patch "prep changes". > > - allow invalid op emulation for kernel mode also. > > - Use CR0_READ_SHADOW instead of GUEST_CR0. > > > > Signed-off-by: Mukesh Rathor > > Acked-by: Keir Fraser > > Reviewed-by: Andrew Cooper > > PV-HVM-Regression-Tested-by: Andrew Cooper > > > > Again the changes above void the tags here. > > > +static int vmxit_msr_read(struct cpu_user_regs *regs) > > +{ > > + u64 msr_content = 0; > > + > > + switch ( regs->ecx ) > > Did you mean regs->_ecx? Hmm.. don't understand why? HVM uses ecx: hvm_msr_read_intercept(regs->ecx, &msr_content) == X86EMUL_OKAY ) > > + default: > > + /* PVH fixme: see hvm_msr_read_intercept(). */ > > + rdmsrl(regs->ecx, msr_content); > > So what does this comment refer to? There's no change to the > referred to function here. And it seems rather questionable that > reading the physical MSR values for everything but > MSR_IA32_MISC_ENABLE is correct/secure. I appreciate the > "fixme" annotation, but I'm afraid this is not sufficient here. Yes, it needs to be revisited, best with AMD port so that a good solution can be contrived for PVH. > > +{ > > + int vector = (__vmread(VM_EXIT_INTR_INFO)) & > > INTR_INFO_VECTOR_MASK; > > + int rc = -ENOSYS; > > + > > + dbgp1(" EXCPT: vec:%#x cs:%#lx rip:%#lx\n", vector, > > + __vmread(GUEST_CS_SELECTOR), regs->eip); > > Do you continue to have these funny dbgp constructs in here. Are > they supposed to go away before this gets committed? If not, > please use a model similar to HVM_DBG_LOG(). Like the commit log says, it helps debug, but can be removed anytime. I left it there thinking it might be useful for first couple months while it gets thoroughly tested. > > +static int vmxit_io_instr(struct cpu_user_regs *regs) > > +{ > > + struct segment_register seg; > > + int requested = (regs->rflags & X86_EFLAGS_IOPL) >> 12; > > + int curr_lvl = (regs->rflags & X86_EFLAGS_VM) ? 3 : 0; > > + > > + if ( curr_lvl == 0 ) > > + { > > + hvm_get_segment_register(current, x86_seg_ss, &seg); > > + curr_lvl = seg.attr.fields.dpl; > > + } > > + if ( requested >= curr_lvl && emulate_privileged_op(regs) ) > > + return 0; > > + > > + hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code); > > I don't think reg->error_code is valid here, I think this needs to be > read from the VMCS. Correct. Thats a bug. thanks Mukesh