Re: [PATCH 06/18] PVH xen: Introduce PVH guest type and some basic changes.

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

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