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

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 01 May 2013 14:52:27 +0100
"Jan Beulich" <jbeulich@suse.com> wrote:

> >>> Mukesh Rathor <mukesh.rathor@oracle.com> 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. 

> >> > +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.
Alternate would be to add a new return code:  EXCRET_inject_pf.

thanks
Mukesh