From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mukesh Rathor <mukesh.rathor@oracle.com>
Subject: Re: [PATCH 06/18] PVH xen: Introduce PVH guest type and
 some basic changes.
Date: Tue, 25 Jun 2013 18:14:18 -0700
Message-ID: <20130625181418.577b6483@mantra.us.oracle.com>
References: <1372118507-16864-1-git-send-email-mukesh.rathor@oracle.com>
	<1372118507-16864-7-git-send-email-mukesh.rathor@oracle.com>
	<51C9788302000078000E0408@nat28.tlf.novell.com>
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: <51C9788302000078000E0408@nat28.tlf.novell.com>
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: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On Tue, 25 Jun 2013 10:01:23 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -644,6 +644,13 @@ int arch_set_info_guest(
> >      unsigned int i;
> >      int rc = 0, compat;
> >  
> > +    /* This removed when all patches are checked in and PVH is
> > done. */
> > +    if ( is_pvh_vcpu(v) )
> > +    {
> > +        printk("PVH: You don't have the correct xen version for
> > PVH\n");
> > +        return -EINVAL;
> > +    }
> > +
> 
> As the patch doesn't add code setting guest_type to is_pvh, this is
> pointless to be added here. The only logical thing, if at all, would
> be an ASSERT().

Actually, now that dom0 and tools are out, one can't create a PVH guest
anyways. My intention was to disallow creation till we reach a satisfactory
point in patches. So, I can just move this to toolset/dom0 whichever
patchset is next.


> > --- a/xen/include/asm-x86/desc.h
> > +++ b/xen/include/asm-x86/desc.h
> > @@ -38,7 +38,13 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > +#ifndef NDEBUG
> > +/* PVH 32bitfixme : see emulate_gate_op call from
> > do_general_protection */ +#define GUEST_KERNEL_RPL(d)
> > (is_pvh_domain(d) ? ({ BUG(); 0; }) :    \
> > +
> > is_pv_32bit_domain(d) ? 1 : 3) +#else
> >  #define GUEST_KERNEL_RPL(d) (is_pv_32bit_domain(d) ? 1 : 3)
> > +#endif
> 
> As it is easily doable, please without explicit check of NDEBUG. E.g.
> 
> #define GUEST_KERNEL_RPL(d) ({ ASSERT(!is_pvh_domain(d)); \
>                                                 is_pv_32bit_domain(d) ?
> 1 : 3; })

OK, thanks.

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -238,6 +238,14 @@ struct mem_event_per_domain
> >      struct mem_event_domain access;
> >  };
> >  
> > +/*
> > + * PVH is a PV guest running in an HVM container. While is_hvm_*
> > checks are
> > + * false for it, it uses many of the HVM data structs.
> > + */
> > +enum guest_type {
> > +    is_pv, is_pvh, is_hvm
> 
> Pretty odd names for enumerators - it's more conventional for them
> to have a prefix identifying their enumeration type in some way.

Ok, which is better: 

         guest_is_pv, guest_is_pvh, guest_is_hvm
or
         guest_type_is_pv, guest_type_is_pvh, guest_type_is_hvm
or
         dom_is_pv, dom_is_pvh, dom_is_hvm    (change enum to domain_type)

thanks,
Mukesh