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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-07-30 at 16:47 -0700, Mukesh Rathor wrote:
> On Mon, 17 Jun 2013 12:11:34 +0100
> Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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?

I think there's a bit of confusion because the current libxc interface
is there to support user-driven direct override of the required feature
flags. IOW a user literally writes "features = FOO" in their config file
and that ends up being f_requested. Although libxl supports this concept
it is not plumbed into xl, I don't know/care what xend does either.

In any case this is not the use case you are looking for. What we want
for PVH is for libxc internally to decide on a set of features which are
required to launch a domain with a specific configuration, in this case
PVH. That's slightly orthogonal to the existing stuff. This isn't
something which has come up yet and so PVH will be the first to go down
this path, which is why you aren't finding all the necessary bits there
out of the box.

I suspect it would be sufficient for libxc (likely xc_dom_allocate) to
call elf_xen_parse_features a second time (or first if features == NULL)
to union the required features into f_requested. You might also need to
blacklist features which PVH is not comfortable with and error out if
the user asked for them at the appropriate time. You will need to do
something similar for kernels which declare a requirement for a feature
which PVH doesn't coexist with (are there any such XENFEAT_*?).

Actually, I think you might want to add a second array of f_required,
that is the set of features which absolutely have to be there and plumb
that down. This corresponds to the third parameter to
elf_xen_parse_features which is currently unused at the xc_dom_allocate
call site. The distinction probably becomes relevant when you support
pvh=no|yes|ifpossible? IOW if yes then the features are required, if
just ifpossible then they are only requested. Not sure, hopefully it
will come out in the wash.

Or maybe it actually makes sense to separate out the user requested
f_{requested,required} field from the libxc internal feature
preferences/requirements. I'm not sure. I'd probably start by reusing
the f_foo ones but if that becomes unwieldy because you find yourself
needing to know whose preference it is then back off into using a
separate pair of fields.

I'm not sure how you are currently signalling to the hypervisor that a
new domain is a PVH domain? I had a look through this patch and must be
being thick because I don't see it.

Anyway, at the point where you set that then you can check the set of
features which the kernel actually ended up supporting vs. those
required to enable PVH and determine if it is actually to enable PVH or
not. I suppose handling the yes vs ifpossible cases might mean two
checks. if yes then maybe in xc_dom_parse_image with the other feature
checks, if it is "ifpossible" I guess it will be near whatever point you
inform the hypervisor that this is to be a PVH domain?

I hope that helps.

Ian.


> 
> thanks
> mukesh



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