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

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

On Wed, 31 Jul 2013 13:00:57 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

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