From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH 08/18] PVH xen: tools changes to create PVH domain Date: Fri, 14 Jun 2013 17:14:37 -0700 Message-ID: <20130614171437.49f55cea@mantra.us.oracle.com> References: <1369445137-19755-1-git-send-email-mukesh.rathor@oracle.com> <1369445137-19755-9-git-send-email-mukesh.rathor@oracle.com> <1371049088.24512.450.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1371049088.24512.450.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "Xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On Wed, 12 Jun 2013 15:58:08 +0100 Ian Campbell 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