Re: [Xen-devel] [PATCH 10/17] PVH xen: introduce vmx_pvh.c and pvh.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.13 at 03:10, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Wed, 01 May 2013 14:52:27 +0100
> "Jan Beulich" <jbeulich@xxxxxxxx> wrote:
> 
>> >>> Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel