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

["Jan Beulich" <jbeulich@suse.com>]

>>> Mukesh Rathor <mukesh.rathor@oracle.com> 05/01/13 2:51 AM >>>
>On Wed, 24 Apr 2013 09:47:55 +0100
>"Jan Beulich" <JBeulich@suse.com> wrote:
>> >>> On 23.04.13 at 23:25, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >>> wrote:
>> > @@ -1503,7 +1503,8 @@ void vmx_do_resume(struct vcpu *v)
>> >  
>> >          vmx_clear_vmcs(v);
>> >          vmx_load_vmcs(v);
>> > -        if ( !is_pvh_vcpu(v) ) {
>> > +        if ( !is_pvh_vcpu(v) )
>> > +        {
>> 
>> Surely an unnecessary adjustment, if an earlier patch got it right
>> from the beginning?
>
>Hmm... I don't understand lot of the time code, but PVH uses PV time
>ops right now, so don't need to worry about it. But the time thing needs
>a revisit anyways with more vtsc modes added in phase II.

The point was that this is a formatting only change, which you shouldn't
do here, but get things right where you insert the conditional.

>> > +    };
>> > +
>> > +    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.

>> > +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.

>> > +static int vmxit_io_instr(struct cpu_user_regs *regs)
>> > +{
>> > +    int curr_lvl;
>> > +    int requested = (regs->rflags >> 12) & 3;
>> > +
>> > +    read_vmcs_selectors(regs);
>> > +    curr_lvl = regs->cs & 3;
>> 
>> Shouldn't you look at SS'es DPL instead?
>
>Ok. It looks like CPL is stored in both CS and SS, so either
>should be ok. But I changed it to ss. 

Your response reads as if you're still looking at the low two bits of the selector,
whereas me using DPL was intended to hint at you needing to look at the "hidden"
portion of the register.

>> > +    switch ( (uint16_t)exit_reason )
>> > +    {
>> > +        case EXIT_REASON_EXCEPTION_NMI:      /* 0 */
>> > +            rc = vmxit_exception(regs);
>> > +            break;
>> 
>> Why would an NMI be blindly reflected to the guest?
>
>I wish it was named EXIT_REASON_EXCEPTION_OR_NMI.
>Anyways, TRAP_machine_check is handled in caller. We handle other 
>excpetions here.

Okay, but then please add a comment to that effect.
 
Jan