Re: [V10 PATCH 23/23] PVH xen: introduce vmexit handler for PVH

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

On Mon, 12 Aug 2013 17:00:36 +0100
George Dunlap <dunlapg@umich.edu> wrote:

> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
....
> > Changes in V8:
> >   - Mainly, don't read selectors on vmexit. The macros now come to
> > VMCS to read selectors on demand.
> 
> Overall I have the same comment here as I had for the VMCS patch: the
> code looks 98% identical.  Substantial differences seem to be:
>  - emulation of privileged ops
>  - cpuid
>  - cr4 handling
> 
> It seems like it would be much better to share the codepath and just
> put "is_pvh_domain()" in the places where it needs to be different.

Depends, and again could be argued either way. The intercepts are so few
for PVH that having a lightweight external handler makes it much easier
to follow and debug. Also, PVH doesn't carry lot of the baggage of HVM,
given we require HAP. Other maintainers I asked had also suggested making
it a separate function.

> > ((uint64_t)regs->edx << 32); +
> > +    dbgp1("PVH: msr write:0x%lx. eax:0x%lx edx:0x%lx\n", regs->ecx,
> > +          regs->eax, regs->edx);
> > +
> > +    if ( hvm_msr_write_intercept(regs->ecx, msr_content) ==
> > X86EMUL_OKAY )
> > +    {
> > +        vmx_update_guest_eip();
> > +        return 0;
> > +    }
> > +    return 1;
> > +}
> > +
> > +static int vmxit_debug(struct cpu_user_regs *regs)
> > +{
> > +    struct vcpu *vp = current;
> > +    unsigned long exit_qualification =
> > __vmread(EXIT_QUALIFICATION); +
> > +    write_debugreg(6, exit_qualification | 0xffff0ff0);
> > +
> > +    /* gdbsx or another debugger. Never pause dom0. */
> > +    if ( vp->domain->domain_id != 0 &&
> > vp->domain->debugger_attached )
> > +        domain_pause_for_debugger();
> > +    else
> > +        hvm_inject_hw_exception(TRAP_debug,
> > HVM_DELIVER_NO_ERROR_CODE);
> 
> Hmm, strangely enough, the HVM handler for this doesn't seem to
> deliver this exception -- or if it does, I can't quite figure out
> where.  What you have here seems like the correct thing to do, but I
> would be interested in knowing the reason for the HVM behavior.

HVM doesn't intercept this trap unless MTF is not available. We just
keep things simple for PVH. Incase of MTF, we just won't get here.

..
> > +/* Just like HVM, PVH should be using "cpuid" from the kernel
> > mode. */ +static int vmxit_invalid_op(struct cpu_user_regs *regs)
> > +{
> > +    if ( guest_kernel_mode(current, regs)
> > || !emulate_forced_invalid_op(regs) )
> > +        hvm_inject_hw_exception(TRAP_invalid_op,
> > HVM_DELIVER_NO_ERROR_CODE); +
> > +    return 0;
> > +}
> > +
> > +/* Returns: rc == 0: handled the exception. */
> > +static int vmxit_exception(struct cpu_user_regs *regs)
> > +{
> > +    int vector = (__vmread(VM_EXIT_INTR_INFO)) &
> > INTR_INFO_VECTOR_MASK;
> > +    int rc = -ENOSYS;
> 
> The vmx code here has some handler for faults that happen during a
> guest IRET -- is that an issue for PVH?

Hmmm... possibly! But reading the SDMs on this is making my head spin. 
Lets not hold the series while I investigate this.

> > +            return -EPERM;
> > +        }
> > +        /* TS going from 1 to 0 */
> > +        if ( (old_cr0 & X86_CR0_TS) && ((new_cr0 & X86_CR0_TS) ==
> > 0) )
> > +            vmx_fpu_enter(vp);
> > +
> > +        vp->arch.hvm_vcpu.hw_cr[0] = vp->arch.hvm_vcpu.guest_cr[0]
> > = new_cr0;
> > +        __vmwrite(GUEST_CR0, new_cr0);
> > +        __vmwrite(CR0_READ_SHADOW, new_cr0);
> > +    }
> > +    else
> > +        *regp = __vmread(GUEST_CR0);
> 
> The HVM code here just uses hvm_vcpu.guest_cr[] -- is there any reason
> not to do the same here?  And in any case, shouldn't it be
> CR0_READ_SHADOW?

They are all the same for PVH.

> > +        if ( !(new & X86_CR4_PAE) && hvm_long_mode_enabled(vp) )
> > +        {
> > +            printk(XENLOG_G_WARNING "Guest cleared CR4.PAE while "
> > +                   "EFER.LMA is set");
> > +            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > +            return 0;
> > +        }
> > +
> > +        vp->arch.hvm_vcpu.guest_cr[4] = new;
> > +
> > +        if ( (old_val ^ new) & (X86_CR4_PSE | X86_CR4_PGE |
> > X86_CR4_PAE) )
> > +            vpid_sync_all();
> 
> Is it actually allowed for a PVH guest to change modes like this?

The 64bit guest should only change the PGE. 
 
> I realize that at the moment you're only supporting HAP, but that may
> not always be true; would it make sense to call
> paging_update_paging_modes() here instead?

Lets do it in steps. When we support other modes, we can always
update this. Right now, we dont' really keep track of guest CR3 because
we require HAP. Wanting PVH without HAP in future seems extermely low
probability to me at this time. We have lot more work to do for PVH,
like migration etc.. and keeping things simple will only help us IMHO.

> This seems to be a weird way to do things, but I see this is what they
> do in vmx_vmexit_handler() as well, so I guess it makes sense to
> follow suit.
> 
> What about EXIT_REASON_TRIPLE_FAULT?

Would result in domain crash (just like HVM).


thanks
mukesh