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

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

On Wed, 12 Jun 2013 15:58:08 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Fri, 2013-05-24 at 18:25 -0700, Mukesh Rathor wrote:
>---
........
ols/xenstore/xenstored_domain.c |   12 +++++++-----
> 
> I think these should be split into
> 	libxc (dombuilder) changes
> 	libxl changes
> 	xenstore changes
> 	misc other (== gdbsx) changes.
> 
> Since most of the changes here are not mentioned at all in the commit
> message (it was the xenstore change, which IMHO requires an
> explanation, which prompted this)

Ok, I'll just do a separate tools patch, and separate them.

> >  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?


> > 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? 


> > @@ -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.

> >      if (xlu_cfg_replace_string (config, "name", &c_info->name, 0))
> > { fprintf(stderr, "Domain name must be specified.\n");
> >          exit(1);
> > @@ -918,6 +928,7 @@ static void parse_config_data(const char
> > *config_source, 
> >          b_info->u.pv.cmdline = cmdline;
> >          xlu_cfg_replace_string (config, "ramdisk",
> > &b_info->u.pv.ramdisk, 0);
> > +        libxl_defbool_set(&b_info->pvh,
> > libxl_defbool_val(c_info->pvh));
> 
> b_info->pvh = c_info->pvh is the right way to do this. If possible I'd
> like to remove one or the other from and handle it internally to the
> library. As I say I need to chew on this one a bit more.

Ok, please let me know what you come up with.

thanks,
Mukesh