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

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

On Thu, 21 Mar 2013 16:49:12 +0000
Tim Deegan <tim@xen.org> wrote:

> ???
> 
> > +        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(). 

Changed to switch.

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

well, we call vpid_sync_vcpu_gva(), but we can optimize this further
in future.

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

No, lets not embed switch statements please, that is obfuscating.

thanks,
Mukesh