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: Wed, 31 Jul 2013 19:02:13 -0700 Message-ID: <20130731190213.0b57efd0@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> <20130614171437.49f55cea@mantra.us.oracle.com> <1371467494.23802.49.camel@zakaz.uk.xensource.com> <20130730164716.10969419@mantra.us.oracle.com> <1375272057.7382.24.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1375272057.7382.24.camel@kazak.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" , Ian Jackson List-Id: xen-devel@lists.xenproject.org On Wed, 31 Jul 2013 13:00:57 +0100 Ian Campbell wrote: > On Tue, 2013-07-30 at 16:47 -0700, Mukesh Rathor wrote: > > On Mon, 17 Jun 2013 12:11:34 +0100 > > Ian Campbell wrote: ...... > > 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_*?). If libxl is already parsing and supposed to be passing features parameter to xc_dom_allocate(), why can't we just let it set the string for PVH when calling xc_dom_allocate in libxl__build_pv? That way libxc can remain transparent. For tools, PVH is a PV guest with some features like auto-xlate etc.., so the more we hide it, the better IMO. If the answer is still no,. it appears that xc_dom_allocate is the best place to put the feature strings. Since, for PVH, features are pre-determined, features not being NULL would be an error. I can juse use the existing xc_interface_core.flags? (would like to rename it to xc_flags so one can easily find its usages please :)). So: xc_dom_allocate: if (xch->flags & PVH) { if (features) { error return NULL; } features = writable_descriptor_tables|auto_translated_physmap" "|supervisor_mode_kernel|hvm_callback_vector; } if ( features ) elf_xen_parse_features(features, dom->f_requested, NULL); what do you think? Acutally, wait! Looking at code more, I think I found the place we need to put the check. In xc_dom_parse_elf_kernel: After: if ( elf_xen_feature_get(XENFEAT_dom0, dom->parms.f_required) ) { xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does not" " support unprivileged (DomU) operation", __FUNCTION__); rc = -EINVAL; goto out; } Add: if (dom->pvh) { if ( !elf_xen_feature_get(XENFEAT_hvm_callback_vector, dom->parms.f_supported) || !elf_xen_feature_get(XENFEAT_auto_translated_physmap dom->parms.f_supported) || ... { xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does not support PVH"...... rc = -EINVAL; goto out; } } BTW, i think the check should be against f_supported and not f_required. This seems like the best solution to me. Agree? Also, it's pvh=yes/no for now. For experimental phases we don't want if possible. There was a discussion, and a decision IIRC, about just booting PV in PVH mode "if possible" by default in future, but not now when we are in the experimental phase. > 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. I had a flag set, but it was recommended during RFC to remove it. So, now in xen, a PV with HAP is a PVH guest: do_domctl(): if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hvm_guest ) domcr_flags |= DOMCRF_hvm; + else if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap ) + domcr_flags |= DOMCRF_pvh; /* PV with HAP is a PVH guest */ + thanks for your help. Mukesh