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

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

On Mon, 17 Jun 2013 12:11:34 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Fri, 2013-06-14 at 17:14 -0700, Mukesh Rathor wrote:
....
> > > > 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.

Alright, I'm still not able to figure this out. I was able to instrument
libraries to figure what goes on for PV. But, I see for PV both 
dom->f_requested  and dom->parms.f_required is null in xc_dom_parse_image(). 
Also, in the same function dom->parms.f_supported is checked, but I can't 
tell greping for f_supported where it's set! I am using xl and not xm, so
don't care what xm/python is setting. I was expecting to see some
features for PV set in those strings. 

It looks like elf_xen_parse_features sets the feature bits, but it's not 
being called for PV in xc_dom_allocate() because features parameter is 
null. 

So, that brings me back to setting the feature string somewhere before
xc_dom_allocate() is called. I'm at a loss where to set it? The feature
string should be set to following for pvh when xc_dom_allocate() is called:

       "writable_descriptor_tables|auto_translated_physmap"
       "|supervisor_mode_kernel|hvm_callback_vector

and if the kernel elf doesn't provide those features, the create should fail.
libxl__build_pv calls xc_dom_allocate(), but you don't want me to add it
in libxl.

I can just put the damn thing in xc_dom_allocate() if features is NULL,
or re-malloc if feature is not NULL for pvh domain?

thanks
mukesh