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

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

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.

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

    regs->eip += ilen;

#if defined(XEN_KDB_CONFIG)
    if ( kdb_handle_trap_entry(TRAP_int3, regs) )
        return 0;
#endif
    /* gdbsx or another debugger. Never pause dom0 */
    if ( vp->domain->domain_id != 0 && guest_kernel_mode(vp, regs) )
......


> > +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():

   rc == 0 =>  not a valid emul signature. inject #UD.
   rc == 1 && addr != 0 => copy failed, need to inject PF
   rc == 1 && addr == 0 => emul done succesfully 

 
> > +static int access_cr4(struct cpu_user_regs *regs, uint acc_typ,
> > uint64_t *regp) +{
> > +    if ( acc_typ == VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR )
> > +    {
> > +        u64 old_cr4 = __vmread(GUEST_CR4);
> > +
> > +        if ( (old_cr4 ^ (*regp)) & (X86_CR4_PSE | X86_CR4_PGE |
> > X86_CR4_PAE) )
> > +            vpid_sync_all();
> > +
> > +        __vmwrite(GUEST_CR4, *regp);
> 
> No modification of CR4_READ_SHADOW here?

Added. BTW, I think I need to also set following unconditionally: 

     *regp |= X86_CR4_VMXE | X86_CR4_MCE;
     __vmwrite(GUEST_CR4, *regp);

in case the guest is turning them off.
 
> > +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. 

> > +    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.
 
> > +        case EXIT_REASON_CPUID:              /* 10 */
> > +        {
> > +            if ( guest_kernel_mode(vp, regs) )
> > +                pv_cpuid(regs);
> > +            else
> > +                pvh_user_cpuid(regs);
> 
> What's the reason for this distinction? I would think it's actually a
> benefit of PVH to allow also hiding unwanted features from guest
> user mode (like HVM, but unlike PV without CPUID faulting).

I was trying to keep it exactly as PV where a user mode would not
be trapped. I will just call pv_cpuid() for both then.
 
> > +int vmx_pvh_read_descriptor(unsigned int sel, const struct vcpu *v,
> > +                            const struct cpu_user_regs *regs,
> > +                            unsigned long *base, unsigned long
> > *limit,
> > +                            unsigned int *ar)
> > +{
> > +    unsigned int tmp_ar = 0;
> > +    ASSERT(v == current);
> > +    ASSERT(is_pvh_vcpu(v));
> > +
> > +    if ( sel == (unsigned int)regs->cs )
> > +    {
> > +        *base = __vmread(GUEST_CS_BASE);
> > +        *limit = __vmread(GUEST_CS_LIMIT);
> > +        tmp_ar = __vmread(GUEST_CS_AR_BYTES);
> > +    }
> > +    else if ( sel == (unsigned int)regs->ds )
> 
> This if/else-if sequence can't be right - a selector can be in more
> than one selector register (and one of them may have got reloaded
> after a GDT/LDT adjustment, while another may not), so you can't
> base the descriptor read upon the selector value. The caller will
> have to tell you which register it wants the descriptor for, not which
> selector.

Ah, right! Duh. I must have made the change same time as the read_sreg
macro.
 
thanks a lot for your time.
Mukesh