From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 10/17] PVH xen: introduce vmx_pvh.c and pvh.c Date: Thu, 02 May 2013 07:42:16 +0100 Message-ID: <518226E802000078000D26CC@nat28.tlf.novell.com> References: <1366752366-16594-1-git-send-email-mukesh.rathor@oracle.com> <1366752366-16594-11-git-send-email-mukesh.rathor@oracle.com> <5177B85B02000078000D03CA@nat28.tlf.novell.com> <20130430175130.7cd4a1e5@mantra.us.oracle.com> <51812C2B0200007800099AF7@nat28.tlf.novell.com> <20130501181028.3d321a8f@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130501181028.3d321a8f@mantra.us.oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 02.05.13 at 03:10, Mukesh Rathor wrote: > On Wed, 01 May 2013 14:52:27 +0100 > "Jan Beulich" wrote: > >> >>> Mukesh Rathor 05/01/13 2:51 AM >>> >> >On Wed, 24 Apr 2013 09:47:55 +0100 >> >> > + regs->eip += ilen; >> >> > + >> >> > + /* gdbsx or another debugger. Never pause dom0 */ >> >> > + if ( vp->domain->domain_id != 0 && guest_kernel_mode(vp, >> >> > regs) ) >> >> > + { >> >> > + dbgp1("[%d]PVH: domain pause for debugger\n", >> >> > smp_processor_id()); >> >> > + current->arch.gdbsx_vcpu_event = TRAP_int3; >> >> > + domain_pause_for_debugger(); >> >> > + return 0; >> >> > + } >> >> > + >> >> > + regs->eip -= ilen; >> >> >> >> Please move the first adjustment into the if() body, making the >> >> second adjustment here unnecessary. >> > >> >Actually, there could more debuggers being used also, so if you don't >> >mind i'd like to leave it as is: >> >> I do mind actually, not the least because I even consider it wrong to >> do the adjustment before calling out to the debugger code. That code >> should be handed the original state, not something already modified. > > In case of non-vmx, upon int3, the eip is advanced to eip+1, where in > case of vmx, eip is not. (I mean in vmx eip is pointing to CC where > in non-vmx it's pointing to eip+1 upon exception). Most debuggers will > check if (eip-1 == breakpoint addr) and take ownership if it is. INT3 being a trap rather than a fault has always been looking odd to me, and in all debugger code I ever wrote I always adjusted for that first thing in the handler (accepting or working around the issue with the exception potentially having been caused by CD 03 instead of CC). >> >> > +static int vmxit_invalid_op(struct cpu_user_regs *regs) >> >> > +{ >> >> > + ulong addr = 0; >> >> > + >> >> > + if ( guest_kernel_mode(current, regs) || >> >> > + emulate_forced_invalid_op(regs, &addr) == 0 ) >> >> > + { >> >> > + hvm_inject_hw_exception(TRAP_invalid_op, >> >> > HVM_DELIVER_NO_ERROR_CODE); >> >> > + return 0; >> >> > + } >> >> > + if ( addr ) >> >> > + hvm_inject_page_fault(0, addr); >> >> >> >> This cannot be conditional upon addr being non-zero. >> > >> >Why not? rc = emulate_forced_invalid_op(): >> >> Because zero can be a valid address that a fault occurred on. > > Hmm... for that to happen, the guest would have to cause vmxit > with invalid op at address 000H. I didn't think that was possible. Why would it not. You have to cover all guest kernels, and not misbehave on malicious ones (i.e. those ought to get an exception injected if so needed, no matter what address it occurred on). > Alternate would be to add a new return code: EXCRET_inject_pf. Something along those lines, yes. Jan