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

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

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

> On Thu, 16 Aug 2012, Mukesh Rathor 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. 
> > 
> > > > +/* 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?
> 
> I am not particularly fussed about the location of SIF_IS_PVINHVM.
> We could define it in asm/xen/hypervisor.h for example.
> 
> The very important bit is to avoid xen_pvh_domain() in generic code
> because it reduces code reusability.
> xen_pvh_domain() covers too many different concepts (a PV guest, in an
> HVM container, using nested paging in hardware), if we bundle them
> together it makes it much harder to reuse them.
> So we should avoid checking for xen_pvh_domain() and check for the
> relevant sub-property we actually interested in.
> 
> For example in balloon.c we are probably only interested in memory
> related behavior, so checking for XENFEAT_auto_translated_physmap
> should be enough.  In other parts of the code we might want to check
> for xen_pv_domain(). If xen_pv_domain() and
> XENFEAT_auto_translated_physmap are not enough, we could introduce
> another small XENFEAT that specifies that the domain is running in a
> HVM container. This way they are all reusable.

yeah, I thought about that, but wasn't sure what the implications would
be for a guest thats not PVH but has auto xlated physmap, if there's
such a possibility. If you guys think thats not an issue, I can change
it.