From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mukesh Rathor <mukesh.rathor@oracle.com>
Subject: Re: [PATCH 10/18 V2]: PVH xen: introduce vmx_pvh.c and
 pvh.c
Date: Tue, 2 Apr 2013 18:37:34 -0700
Message-ID: <20130402183734.16b1ba68@mantra.us.oracle.com>
References: <20130315174145.4bc0e78f@mantra.us.oracle.com>
	<20130321164912.GO12338@ocelot.phlegethon.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20130321164912.GO12338@ocelot.phlegethon.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tim Deegan <tim@xen.org>
Cc: "Xen-devel@lists.xensource.com" <Xen-devel@lists.xensource.com>
List-Id: xen-devel@lists.xenproject.org

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