Re: [PATCH 08/18] PVH xen: tools changes to create PVH domain

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2013-06-14 at 17:14 -0700, Mukesh Rathor wrote:

> > >  10 files changed, 53 insertions(+), 11 deletions(-)
> > 
> > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > > index f1be43b..24f6759 100644
> > > --- a/tools/libxc/xc_dom_x86.c
> > > +++ b/tools/libxc/xc_dom_x86.c
> > > @@ -832,7 +833,7 @@ int arch_setup_bootlate(struct xc_dom_image
> > > *dom) }
> > >  
> > >          /* Map grant table frames into guest physmap. */
> > > -        for ( i = 0; ; i++ )
> > > +        for ( i = 0; !dom->pvh_enabled; i++ )
> > 
> > This is a bit of an odd way to do this (unless pvh_enabled somehow
> > changes in this loop, which I doubt). Can we just get a surrounding if
> > please.
> 
> Sure (will indent more tho). Are you ok with a forward goto?

Not madly keen. I'd rather pull that loop out into a function.

> 
> 
> > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > index b38d0a7..cefbf76 100644
> > > --- a/tools/libxl/libxl_dom.c
> > > +++ b/tools/libxl/libxl_dom.c
> > > @@ -329,9 +329,23 @@ int libxl__build_pv(libxl__gc *gc, uint32_t
> > > domid, struct xc_dom_image *dom;
> > >      int ret;
> > >      int flags = 0;
> > > +    int is_pvh = libxl_defbool_val(info->pvh);
> > >  
> > >      xc_dom_loginit(ctx->xch);
> > >  
> > > +    if (is_pvh) {
> > > +        char *pv_feats =
> > > "writable_descriptor_tables|auto_translated_physmap"
> > > +
> > > "|supervisor_mode_kernel|hvm_callback_vector"; +
> > > +        if (info->u.pv.features && info->u.pv.features[0] != '\0')
> > > +        {
> > > +            LOG(ERROR, "Didn't expect info->u.pv.features to
> > > contain string\n");
> > > +            LOG(ERROR, "String: %s\n", info->u.pv.features);
> > > +            return ERROR_FAIL;
> > > +        }
> > > +        info->u.pv.features = strdup(pv_feats);
> > 
> > What is this trying to achieve? I think the requirement for certain
> > features to be present if pvh is enabled needs to be handled in the
> > xc_dom library and not here. This field is (I think) for the user to
> > specify other features which they may wish to require.
> 
> I had asked for assitance on this long ago. But anyways, basically here
> I want to make sure the kernel has all those features because the user
> has asked a PVH guest must be created (by pvh=1 in vm.cfg file). Can you 
> kindly advise the best way to do this? 

This should be done in xc_dom build stuff not in libxl. Basically libxl
should call xc_dom_foo with a kernel and pvh=yes (or =ifpossible) and
the builder is then responsible internally for knowing which features
are therefore required from the kernel.

> > > @@ -245,6 +245,7 @@ libxl_domain_create_info =
> > > Struct("domain_create_info",[ ("platformdata",
> > > libxl_key_value_list), ("poolid",       uint32),
> > >      ("run_hotplug_scripts",libxl_defbool),
> > > +    ("pvh",          libxl_defbool),
> > >      ], dir=DIR_IN)
> > >  
> > >  MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
> > > @@ -346,6 +347,7 @@ libxl_domain_build_info =
> > > Struct("domain_build_info",[ ])),
> > >                   ("invalid", Struct(None, [])),
> > >                   ], keyvar_init_val =
> > > "LIBXL_DOMAIN_TYPE_INVALID")),
> > > +    ("pvh",       libxl_defbool),
> > 
> > I'm not quite convinced if the need for both of these bools in both
> > create and build, it's a bit of an odd quirk in our API which I need
> > to consider a bit deeper.
> 
> Ok, please let me know.

Which places need the one in c_info and which the one in b_info?

c_info is presumably for the createdomain domctl call while b_info is
stuff spread around the build process to handle the various differences?

Perhaps libxl__domain_create_state is the right place for the b_info
one, initialised internally to libxl from the c_info one? In that
context it would be a straight bool and not a defbool since we would
know exactly what the domain was by this point.

An alternative would be for the code to query the domain's type (i.e.
from the hypervisor) when it needs to know -- e.g. libxl__domain_type
does this for HVM vs PV.

Ian.