Re: [Xen-devel] [RFC PATCH 10/16]: PVH xen: introduce vmx_pvh.c

[Tim Deegan <tim@xxxxxxx>]

At 18:01 -0800 on 11 Jan (1357927270), Mukesh Rathor wrote:
> The heart of this patch is vmx exit handler for PVH guest. It is nicely
> isolated in a separate module. A call to it is added to
> vmx_pvh_vmexit_handler().

Not looking at this in fine detail yet (considering the various 'fixme's
still around in it), but one or two things:


> +static noinline int vmxit_exception(struct cpu_user_regs *regs)
> +{
> +    unsigned int vector = (__vmread(VM_EXIT_INTR_INFO)) & 
> INTR_INFO_VECTOR_MASK;
> +    int rc=1; 
> +    struct vcpu *vp = current;
> +
> +    dbgp2(" EXCPT: vec:%d cs:%lx r.IP:%lx\n", vector, 
> vmr(GUEST_CS_SELECTOR), 
> +          regs->eip);
> +
> +    if (vector == TRAP_debug) {
> +        unsigned long exit_qualification = __vmread(EXIT_QUALIFICATION);
> +        write_debugreg(6, exit_qualification | 0xffff0ff0);
> +        regs->rip = vmr(GUEST_RIP); 
> +        regs->rsp = vmr(GUEST_RSP);
> +        rc = 0;
> +        
> +     /* gdbsx or another debugger */

This is a hard tab.  Actually, the whitespace in this file needs
attention generally.

> +void vmx_pvh_vmexit_handler(struct cpu_user_regs *regs)
> +{
> +    unsigned long exit_qualification;
> +    unsigned int vector, exit_reason = __vmread(VM_EXIT_REASON);
> +    int rc=0, ccpu = smp_processor_id();
> +    struct vcpu *vp = current;
> +
> +    dbgp1("PVH:[%d]left VMCS exitreas:%d RIP:%lx RSP:%lx EFLAGS:%lx 
> CR0:%lx\n", 
> +          ccpu, exit_reason, vmr(GUEST_RIP), vmr(GUEST_RSP), regs->rflags, 
> +          vmr(GUEST_CR0));
> +
> +    /* for guest_kernel_mode() */
> +    regs->cs = vmr(GUEST_CS_SELECTOR); 
> +
> +    switch ( (uint16_t)exit_reason )
> +    {
> +        case EXIT_REASON_EXCEPTION_NMI:
> +        case EXIT_REASON_EXTERNAL_INTERRUPT:
> +        case EXIT_REASON_MCE_DURING_VMENTRY:
> +            break;
> +        default:
> +            local_irq_enable();
> +    }

That's a bit risky: EXIT_REASON_EXCEPTION_NMI includes a lot of cases
that might not be safe to handle with interrupts disabled.  Also I think
it means there are paths through this function that don't enable irqs at
all.

I think it'd be better to do it the way vmx_vmexit_handler() does:
explicitly sort out the things that _must_ be done with irqs disabled
first, so it's clear which code runs with irqs enabled and which doesn't.

> +    switch ( (uint16_t)exit_reason )
> +    {
> +        case EXIT_REASON_EXCEPTION_NMI:      /* 0 */
> +            rc = vmxit_exception(regs);
> +            break;
> +            
> +        case EXIT_REASON_EXTERNAL_INTERRUPT: /* 1 */
> +        {
> +            vector = __vmread(VM_EXIT_INTR_INFO);
> +            vector &= INTR_INFO_VECTOR_MASK;
> +            vmx_do_extint(regs);
> +            break;
> +        }
> +
> +        case EXIT_REASON_TRIPLE_FAULT:  /* 2 */
> +        {
> +            dbgp0("PVH:Triple Flt:[%d]exitreas:%d RIP:%lx RSP:%lx EFLAGS:%lx 
> CR3:%lx\n",
> +                  ccpu, exit_reason, vmr(GUEST_RIP), vmr(GUEST_RSP), 
> +                  regs->rflags, vmr(GUEST_CR3));
> +
> +            vp->arch.hvm_vcpu.guest_cr[3] = vp->arch.hvm_vcpu.hw_cr[3] =
> +                                                           
> __vmread(GUEST_CR3);
> +            rc = 1;
> +            break;
> +        }
> +        case EXIT_REASON_PENDING_VIRT_INTR:  /* 7 */
> +        {
> +            struct vcpu *v = current;
> +            /* Disable the interrupt window. */
> +            v->arch.hvm_vmx.exec_control &= ~CPU_BASED_VIRTUAL_INTR_PENDING;
> +            __vmwrite(CPU_BASED_VM_EXEC_CONTROL, 
> v->arch.hvm_vmx.exec_control);
> +            break;
> +        }
> +
> +        case EXIT_REASON_CPUID:              /* 10 */
> +        {
> +            if ( guest_kernel_mode(vp, regs) ) {
> +                pv_cpuid(regs);
> +
> +                /* Because we are setting CR4.OSFXSR to 0, we need to disable
> +                 * this because, during boot, user process "init" (which 
> doesn't
> +                 * do cpuid), will do 'pxor xmm0,xmm0' and cause #UD. For 
> now 
> +                 * disable this. HVM doesn't allow setting of CR4.OSFXSR.
> +                 * fixme: this and also look at CR4.OSXSAVE */
> +
> +                __clear_bit(X86_FEATURE_FXSR, &regs->edx);

Shouldn't this be gated on which leaf the guest asked for?

Tim.

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