From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 08/18] PVH xen: tools changes to create PVH domain Date: Wed, 31 Jul 2013 13:00:57 +0100 Message-ID: <1375272057.7382.24.camel@kazak.uk.xensource.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> <20130614171437.49f55cea@mantra.us.oracle.com> <1371467494.23802.49.camel@zakaz.uk.xensource.com> <20130730164716.10969419@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130730164716.10969419@mantra.us.oracle.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: Mukesh Rathor Cc: "Xen-devel@lists.xensource.com" , Ian Jackson List-Id: xen-devel@lists.xenproject.org On Tue, 2013-07-30 at 16:47 -0700, Mukesh Rathor wrote: > On Mon, 17 Jun 2013 12:11:34 +0100 > Ian Campbell 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