|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 08/18] PVH xen: tools changes to create PVH domain
On Wed, 2013-07-31 at 19:02 -0700, Mukesh Rathor wrote:
> On Wed, 31 Jul 2013 13:00:57 +0100
> Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxxxx> 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?
Because I think that is the wrong layer.
In any case libxl isn't parsing any features, it is passing a user
provided string through uninspected, libxl doesn't know what those
features mean.
> 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.
Yes, that's my point. Forcing libxl to set a string for PVH when calling
xc_dom_allocate is not making libxc transparent or hiding things, it's
the opposite.
> 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 think it is valid for a user to both request PVH mode and request some
features of their own which they want to enable for this guest. The code
should be set up to deal with this, even if that currently means
rejecting any user request for a feature not in the PVH set (although
I'd prefer to see a stronger rationale for rejecting a requested feature
than that).
> 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 :)).
Pointless churn. You've made this argument countless times about various
and been told no by several different people, please just drop it.
> So:
>
> xc_dom_allocate:
>
> if (xch->flags & PVH)
> {
> if (features)
> {
> error
> return NULL;
No, please handle the case of the user asking for features.
At the very least if they only ask for things in the set you are forcing
then it is fine.
> }
> features = writable_descriptor_tables|auto_translated_physmap"
> "|supervisor_mode_kernel|hvm_callback_vector;
Don't do this. Instead, drop this whole if and add below:
> }
> if ( features )
> elf_xen_parse_features(features, dom->f_requested, NULL);
if ( xch->flags & PVH )
elf_xen_parse_features("writable_desc...|etc", dom->f_requested,
dom->f_required)
I'm surprised that this features are only requested for PVH and not
required (more on this below).
I see PVH is a flag, are you not intending to handle it as a tristate
i.e.:
PVH = no : do not use PVH ever
PVH = yes : either enable PVH or fail
PVH = default : libxc decides if PVH is possible, based on
kernels supported feature set.
Long term "default" should be the, err, default e.g. so that people can
boot PVH or PV kernels via pygrub at their whim.
(I see you've just answered this below, nevermind)
> 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;
> }
> }
This is the sort of thing I was imagining, it's obviously going to need
to be more complex if you want to support optional PVH though.
> BTW, i think the check should be against f_supported and not f_required.
> This seems like the best solution to me. Agree?
I think you may need to separate out the concept of "required/supported
by the kernel" from "required/supported by the domain configuration" in
your thinking (if not the code). This is why I was suggesting you might
need to leave the exising fields for the kernel supported set and have a
separate pair of fields for the set required by the configuration -- I'm
starting to think that make sense, especially once you start to consider
the PVH = default case.
> 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.
Hrm, at the libxc API level this is acceptable (since we can evolve that
API) but it needs to be a libxl_defbool at the libxl level because we
cannot change that once it goes in (not easily at least). This should
just be a case of libxl_defbool_setdefault(false) in the appropriate
setdefaults function in libxl.
> > 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:
Ah, that makes sense.
Ian.
>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |