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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel