Re: [RFC PATCH 1/8]: PVH: Basic and preparatory changes

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

On Fri, 17 Aug 2012 11:56:43 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> On Fri, 17 Aug 2012, Ian Campbell wrote:
> > > > > diff --git a/include/xen/interface/xen.h
> > > > > b/include/xen/interface/xen.h index 0801468..1d5bc36 100644
> > > > > --- a/include/xen/interface/xen.h
> > > > > +++ b/include/xen/interface/xen.h
> > > > > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> > > > >  /* These flags are passed in the 'flags' field of
> > > > > start_info_t. */ #define SIF_PRIVILEGED    (1<<0)  /* Is the
> > > > > domain privileged? */ #define SIF_INITDOMAIN    (1<<1)  /* Is
> > > > > this the initial control domain? */ +#define
> > > > > SIF_IS_PVINHVM    (1<<4)  /* Is it a PV running in HVM
> > > > > container? */ #define SIF_PM_MASK       (0xFF<<8) /* reserve
> > > > > 1 byte for xen-pm options */ typedef uint64_t cpumap_t;
> > > > 
> > > > I would avoid adding SIF_IS_PVINHVM, an x86 specific concept,
> > > > into a generic xen.h interface file. 
> > 
> > Is PVH actually more like a XENFEAT style thing?
> > 
> > Is there actually anywhere which wants to know specifically about
> > PVH rather than some more specific property which a PVH domain
> > happen to has?
> 
> That's exactly the point.
> 
> 
> > > > > +/* xen_pv_domain check is necessary as start_info ptr is
> > > > > null in HVM. Also,
> > > > > + * note, xen PVH domain shares lot of HVM code */
> > > > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > > > &&                     \
> > > > > +				(xen_start_info->flags &
> > > > > SIF_IS_PVINHVM))
> > > >  
> > > > Also here.
> > > 
> > > Hmm.. I can move '#define xen_pvh_domain()' to x86 header, easy.
> > > But, not sure how to define SIF_IS_PVINHVM then? I could put
> > > SIF_IS_RESVD in include/xen/interface/xen.h, and then do 
> > > #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> > > 
> > > What do you think about that?
> > 
> > Should PVH actually be a new value in the xen_domain_type enum?
> 
> I don't think we should have a xen_domain_type pvh at all.
> If we really need it we should define it as a set of individual
> properties:
> 
> #define xen_pvh_domain() (xen_pv_domain() && \
>                           xen_feature(XENFEAT_auto_translated_physmap)
> && \ xen_have_vector_callback)

No, I had started with enum, too many code unnecessary code changes
then. I like the above #define. It eradicates the need for SIF flag.
I'll see if that works.