Re: [Xen-devel] [PATCH 10/18 V2]: PVH xen: introduce vmx_pvh.c and pvh.c

[Tim Deegan <tim@xxxxxxx>]

> +/* Returns: rc == 0: handled the exception/NMI */
> +static 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, 
> +          __vmread(GUEST_CS_SELECTOR), regs->eip);
> +
> +    if (vector == TRAP_debug) {

Missing spaces (here and below) 

> +        unsigned long exit_qualification = __vmread(EXIT_QUALIFICATION);
> +        write_debugreg(6, exit_qualification | 0xffff0ff0);
> +        rc = 0;
> +        
> +        /* gdbsx or another debugger */
> +        if ( vp->domain->domain_id != 0 &&    /* never pause dom0 */
> +             guest_kernel_mode(vp, regs) &&  vp->domain->debugger_attached )
> +        {
> +            domain_pause_for_debugger();
> +        } else {
> +            hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
> +        }
> +    } 
> +    if (vector == TRAP_int3) {
> +        rc = vmxit_int3(regs);
> +
> +    }  else if (vector == TRAP_invalid_op) {
> +        rc = vmxit_invalid_op(regs);
> +
> +    } else if (vector == TRAP_no_device) {
> +        hvm_funcs.fpu_dirty_intercept();  /* calls vmx_fpu_dirty_intercept */
> +        rc = 0;
> +
> +    } else if (vector == TRAP_gp_fault) {
> +        regs->error_code = __vmread(VM_EXIT_INTR_ERROR_CODE);
> +        /* hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code); */

???

> +        rc = 1;
> +
> +    } else if (vector == TRAP_page_fault) {
> +        printk("PVH: Unexpected vector page_fault. IP:%lx\n", regs->eip);
> +        rc = 1;
> +    }

This probably ought to be a switch statement rather than a chain of
'else if'.  Then the single 'default' case would catch all unexpected
exits (AFAICS there are three different variations on that here).  And
since you've set the exception bitmap in the VMCS, presumably the
correct response to an unexpected trap type is to BUG(). 

> +    if (rc)
> +        printk("PVH: Unhandled trap vector:%d. IP:%lx\n", vector, regs->eip);
> +
> +    return rc;
> +}
> +
> +static int vmxit_invlpg(void)
> +{
> +    ulong vaddr = __vmread(EXIT_QUALIFICATION);
> +
> +    vmx_update_guest_eip();
> +    vpid_sync_vcpu_gva(current, vaddr);

If there's no special handling of invlpg, you should just not intercept it. 

> +    return 0;
> +}
>
> +/* Returns: rc == 0: success */
> +static int vmxit_cr_access(struct cpu_user_regs *regs)
> +{
> +    unsigned long exit_qualification = __vmread(EXIT_QUALIFICATION);
> +    uint acc_typ = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
> +    int cr, rc = 1;
> +
> +    switch ( acc_typ )
> +    {
> +        case VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR:
> +        case VMX_CONTROL_REG_ACCESS_TYPE_MOV_FROM_CR:
> +        {
> +            uint gpr = VMX_CONTROL_REG_ACCESS_GPR(exit_qualification);
> +            uint64_t *regp = decode_register(gpr, regs, 0);
> +            cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
> +
> +            if (regp == NULL)
> +                break;
> +
> +            /* pl don't embed switch statements */
> +            if (cr == 0)
> +                rc = access_cr0(regs, acc_typ, regp);
> +            else if (cr == 3) {
> +                printk("PVH: d%d: unexpected cr3 access vmexit. rip:%lx\n", 
> +                       current->domain->domain_id, regs->rip);
> +                domain_crash_synchronous();

Again, ITYM BUG().  And again. probably a switch statement.

Tim.

> +            } else if (cr == 4) 
> +                rc = access_cr4(regs, acc_typ, regp);
> +
> +            if (rc == 0)
> +                vmx_update_guest_eip();
> +            break;
> +        }

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