Re: [PATCH v2 1/3] x86: remove PVHv1 code

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Feb 28, 2017 at 05:44:51PM +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 1/3] x86: remove PVHv1 code"):
> > This removal applies to both the hypervisor and the toolstack side of PVHv1.
> > 
> > Note that on the toolstack side there's one hiccup: on xl the "pvh"
> > configuration option is translated to builder="hvm",
> > device_model_version="none".  This is done because otherwise xl would start
> > parsing PV like options, and filling the PV struct at libxl_domain_build_info
> > (which in turn pollutes the HVM one because it's a union).
> ...
> 
> Thanks.
> 
> I have no argument with the principle, obviously, but I think the
> libxl API needs a bit of adjustment.
> 
> > +=item B<pvh=BOOLEAN>
> > +
> > +Selects whether to run this PV guest in an HVM container. Default is 0.
> > +Note that this option is equivalent to setting builder="hvm" and
> > +device_model_version="none"
> 
> I think this note needs to be reworded or de-emphasised.
> 
> Are those explicit settings even a supported way to create a PVHv2
> domain ?  I think they probably shouldn't be.
> 
> > -    xlu_cfg_get_defbool(config, "pvh", &c_info->pvh, 0);
> > +    if (!xlu_cfg_get_defbool(config, "pvh", &c_info->pvh, 0)) {
> > +        /* NB: we need to set the type here, or else we will fall into
> > +         * the PV path, and the set of options will be completely wrong
> > +         * (even more because the PV and HVM options are inside an union).
> > +         */
> > +        c_info->type = LIBXL_DOMAIN_TYPE_HVM;
> > +        b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_NONE;
> > +    }
> 
> I think this should probably be done in libxl, not xl.

I think Roger's patch is fine.

The "pvh=" option's semantics is changed in xl, so changes should be
made in xl.

(I would go even further to remove the pvh field in IDL because it is
experimental and will serve no particular purpose anymore.)

Wei.

> 
> The rest of this looks OK from a tools pov, AFAICT.
> 
> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel