| Message ID | 20200205165056.11734-7-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools: Rationalise legacy CPUID handling | expand |
On 05.02.2020 17:50, Andrew Cooper wrote: > --- a/tools/libxc/xc_sr_restore_x86_hvm.c > +++ b/tools/libxc/xc_sr_restore_x86_hvm.c > @@ -72,6 +72,16 @@ static int handle_hvm_params(struct xc_sr_context *ctx, > case HVM_PARAM_BUFIOREQ_PFN: > xc_clear_domain_page(xch, ctx->domid, entry->value); > break; > + > + case HVM_PARAM_PAE_ENABLED: > + /* > + * This HVM_PARAM only ever existed a non-standard calling ABI for > + * xc_cpuid_apply_policy(). It has now been updated to use a > + * regular calling convention, making the param obsolete. > + * > + * Discard if we find it in an old migration stream. > + */ > + continue; Having also looked at the previous patch (the only one in this series relevant to the adjustments done here afaict) I wonder whether simply ignoring it (i.e. not even warning anywhere when out of sync with whatever info->u.hvm.pae gets populated from) is a good approach. But of course I may be easily missing aspects here that make this quite fine. > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -86,7 +86,7 @@ > #define HVM_PARAM_STORE_PFN 1 > #define HVM_PARAM_STORE_EVTCHN 2 > > -#define HVM_PARAM_PAE_ENABLED 4 > +#define HVM_PARAM_PAE_ENABLED 4 /* Obsolete. Do not use. */ I think this should be moved up in the deprecated section. With this hypervisor parts Reviewed-by: Jan Beulich <jbeulich@suse.com> As an aside I also think that section should check for just __XEN__, not also __XEN_TOOLS__. Jan
On 06/02/2020 09:28, Jan Beulich wrote: > On 05.02.2020 17:50, Andrew Cooper wrote: >> --- a/tools/libxc/xc_sr_restore_x86_hvm.c >> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c >> @@ -72,6 +72,16 @@ static int handle_hvm_params(struct xc_sr_context *ctx, >> case HVM_PARAM_BUFIOREQ_PFN: >> xc_clear_domain_page(xch, ctx->domid, entry->value); >> break; >> + >> + case HVM_PARAM_PAE_ENABLED: >> + /* >> + * This HVM_PARAM only ever existed a non-standard calling ABI for >> + * xc_cpuid_apply_policy(). It has now been updated to use a >> + * regular calling convention, making the param obsolete. >> + * >> + * Discard if we find it in an old migration stream. >> + */ >> + continue; > Having also looked at the previous patch (the only one in this series > relevant to the adjustments done here afaict) Correct. > I wonder whether simply > ignoring it (i.e. not even warning anywhere when out of sync with > whatever info->u.hvm.pae gets populated from) is a good approach. We can't (easily) cross check at all, because info->u.hvm.pae is in a separate process (as far as the xl/libxl toolstack goes). On cross checking, you'll find that migration in from pre Xen 4.6 doesn't actually have the data. If you look back at the 4.5 legacy migration code, you'll observe that this param is restored but never saved. In hindsight, we probably shouldn't have fixed that in migration v2, but we did. Upstream was actually fine, because libxl sets HVM_PARAM_PAE_ENABLED after the migration stream completes, and overwrites whatever was where. XenServer did not, and we noticed as a consequence of Xen 4.5 actually cross-checked CPUID settings on a mov to %cr4 emulation. > But of course I may be easily missing aspects here that make this quite > fine. It really is obsolete and needs forgetting, not checking. > >> --- a/xen/include/public/hvm/params.h >> +++ b/xen/include/public/hvm/params.h >> @@ -86,7 +86,7 @@ >> #define HVM_PARAM_STORE_PFN 1 >> #define HVM_PARAM_STORE_EVTCHN 2 >> >> -#define HVM_PARAM_PAE_ENABLED 4 >> +#define HVM_PARAM_PAE_ENABLED 4 /* Obsolete. Do not use. */ > I think this should be moved up in the deprecated section. There isn't a deprecated section. The params are currently sorted numerically. Playing "which param is this integer?" with an unsorted params.h is an experience I wish never to repeat again. ~Andrew
On 06.02.2020 12:32, Andrew Cooper wrote: > On 06/02/2020 09:28, Jan Beulich wrote: >> On 05.02.2020 17:50, Andrew Cooper wrote: >>> --- a/xen/include/public/hvm/params.h >>> +++ b/xen/include/public/hvm/params.h >>> @@ -86,7 +86,7 @@ >>> #define HVM_PARAM_STORE_PFN 1 >>> #define HVM_PARAM_STORE_EVTCHN 2 >>> >>> -#define HVM_PARAM_PAE_ENABLED 4 >>> +#define HVM_PARAM_PAE_ENABLED 4 /* Obsolete. Do not use. */ >> I think this should be moved up in the deprecated section. > > There isn't a deprecated section. > > The params are currently sorted numerically. Playing "which param is > this integer?" with an unsorted params.h is an experience I wish never > to repeat again. You'll find /* These parameters are deprecated and their meaning is undefined. */ near the top of the file. I can see your concern about the file not being sorted, but it already isn't. The alternative then is to frame each deprecated param by #ifdef __XEN__ - I'd be okay with that if that's preferred by you. Jan
On 06/02/2020 11:37, Jan Beulich wrote: > On 06.02.2020 12:32, Andrew Cooper wrote: >> On 06/02/2020 09:28, Jan Beulich wrote: >>> On 05.02.2020 17:50, Andrew Cooper wrote: >>>> --- a/xen/include/public/hvm/params.h >>>> +++ b/xen/include/public/hvm/params.h >>>> @@ -86,7 +86,7 @@ >>>> #define HVM_PARAM_STORE_PFN 1 >>>> #define HVM_PARAM_STORE_EVTCHN 2 >>>> >>>> -#define HVM_PARAM_PAE_ENABLED 4 >>>> +#define HVM_PARAM_PAE_ENABLED 4 /* Obsolete. Do not use. */ >>> I think this should be moved up in the deprecated section. >> There isn't a deprecated section. >> >> The params are currently sorted numerically. Playing "which param is >> this integer?" with an unsorted params.h is an experience I wish never >> to repeat again. > You'll find > > /* These parameters are deprecated and their meaning is undefined. */ > > near the top of the file. Urgh - I was looking at the 4.5 code. What's currently in stating is a total mess, and we've got multiple different ways of dealing with reserved values. ~Andrew
Hi Andrew, On 05/02/2020 16:50, Andrew Cooper wrote: > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 76b27c9168..f3426f37fe 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -46,7 +46,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( copy_from_guest(&a, arg, 1) ) > return -EFAULT; > > - if ( a.index >= HVM_NR_PARAMS ) > + if ( a.index >= HVM_NR_PARAMS || > + a.index == HVM_PARAM_PAE_ENABLED ) > return -EINVAL; For the small Arm part: Acked-by: Julien Grall <julien@xen.org>
On 08/02/2020 12:12, Julien Grall wrote: > Hi Andrew, > > On 05/02/2020 16:50, Andrew Cooper wrote: >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index 76b27c9168..f3426f37fe 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -46,7 +46,8 @@ long do_hvm_op(unsigned long op, >> XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( copy_from_guest(&a, arg, 1) ) >> return -EFAULT; >> - if ( a.index >= HVM_NR_PARAMS ) >> + if ( a.index >= HVM_NR_PARAMS || >> + a.index == HVM_PARAM_PAE_ENABLED ) >> return -EINVAL; > > For the small Arm part: > > Acked-by: Julien Grall <julien@xen.org> > So it turns out that, in light of "xen/hvm: Fix handling of obsolete HVM_PARAMs", there is even more breakage here, because it is *only* the libxc code which causes the params to be rejected on ARM. I'm clearly going to have to sink rather more time into this than I was initially intending, so I'll dust off my earlier series and sort this who mess out, once and for all. ~Andrew
Andrew Cooper writes ("[PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED"):
> HVM_PARAM_PAE_ENABLED is undocumented and Xen has never acted upon its value,
> contrary perhaps to expectations based on how other boolean fields work.
>
> It was only ever used as a non-standard calling convention for
> xc_cpuid_apply_policy() but that has been fixed now.
>
> Purge its use, and any possible confusion over its behaviour, by having Xen
> reject any attempts to use it. Forgo setting it up in libxl's
> hvm_set_conf_params(). The only backwards compatibility necessary is to have
> the HVM restore stream discard it if found.
This looks plausible too. But maybe I should be reading this patch
and the previous one together ? Or maybe they would be better
squashed ?
If you think that is likely to make me less confused I'm happy to try
squashing them locally and reading the result...
Ian.
On 11/02/2020 17:49, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED"): >> HVM_PARAM_PAE_ENABLED is undocumented and Xen has never acted upon its value, >> contrary perhaps to expectations based on how other boolean fields work. >> >> It was only ever used as a non-standard calling convention for >> xc_cpuid_apply_policy() but that has been fixed now. >> >> Purge its use, and any possible confusion over its behaviour, by having Xen >> reject any attempts to use it. Forgo setting it up in libxl's >> hvm_set_conf_params(). The only backwards compatibility necessary is to have >> the HVM restore stream discard it if found. > This looks plausible too. But maybe I should be reading this patch > and the previous one together ? Or maybe they would be better > squashed ? > > If you think that is likely to make me less confused I'm happy to try > squashing them locally and reading the result... I don't think that is going to help. They are two logically different changes. Patch 5 fixes a libxl=>libxc api which has a (deliberate) side effect of removing the sole use of HVM_PARAM_PAE_ENABLED. This patch takes the final steps to remove HVM_PARAM_PAE_ENABLED from use, everywhere. This is partly to prevent ever regaining this knobble on the CPUID handling side of things, and eventually to reduce memory usage in Xen (by not allocating memory for obsolete params). ~Andrew
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c index 3f78248f32..da1574ce11 100644 --- a/tools/libxc/xc_sr_restore_x86_hvm.c +++ b/tools/libxc/xc_sr_restore_x86_hvm.c @@ -72,6 +72,16 @@ static int handle_hvm_params(struct xc_sr_context *ctx, case HVM_PARAM_BUFIOREQ_PFN: xc_clear_domain_page(xch, ctx->domid, entry->value); break; + + case HVM_PARAM_PAE_ENABLED: + /* + * This HVM_PARAM only ever existed a non-standard calling ABI for + * xc_cpuid_apply_policy(). It has now been updated to use a + * regular calling convention, making the param obsolete. + * + * Discard if we find it in an old migration stream. + */ + continue; } rc = xc_hvm_param_set(xch, ctx->domid, entry->index, entry->value); diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c index d99efe65e5..7d3f3ddb8f 100644 --- a/tools/libxc/xc_sr_save_x86_hvm.c +++ b/tools/libxc/xc_sr_save_x86_hvm.c @@ -71,7 +71,6 @@ static int write_hvm_params(struct xc_sr_context *ctx) HVM_PARAM_ACPI_IOPORTS_LOCATION, HVM_PARAM_VIRIDIAN, HVM_PARAM_IDENT_PT, - HVM_PARAM_PAE_ENABLED, HVM_PARAM_VM_GENERATION_ID_ADDR, HVM_PARAM_IOREQ_SERVER_PFN, HVM_PARAM_NR_IOREQ_SERVER_PAGES, diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c index 1cae0e2b26..f8bc828e62 100644 --- a/tools/libxl/libxl_x86.c +++ b/tools/libxl/libxl_x86.c @@ -391,12 +391,10 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid, libxl_ctx *ctx = libxl__gc_owner(gc); xc_interface *xch = ctx->xch; int ret = ERROR_FAIL; - bool pae = true, altp2m = info->altp2m; + bool altp2m = info->altp2m; switch(info->type) { case LIBXL_DOMAIN_TYPE_HVM: - pae = libxl_defbool_val(info->u.hvm.pae); - /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For * legacy reasons, both parameters are accepted on x86 HVM guests. * @@ -425,10 +423,6 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid, /* Fallthrough */ case LIBXL_DOMAIN_TYPE_PVH: - if (xc_hvm_param_set(xch, domid, HVM_PARAM_PAE_ENABLED, pae)) { - LOG(ERROR, "Couldn't set HVM_PARAM_PAE_ENABLED"); - goto out; - } if (xc_hvm_param_set(xch, domid, HVM_PARAM_TIMER_MODE, timer_mode(info))) { LOG(ERROR, "Couldn't set HVM_PARAM_TIMER_MODE"); diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 76b27c9168..f3426f37fe 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -46,7 +46,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) if ( copy_from_guest(&a, arg, 1) ) return -EFAULT; - if ( a.index >= HVM_NR_PARAMS ) + if ( a.index >= HVM_NR_PARAMS || + a.index == HVM_PARAM_PAE_ENABLED ) return -EINVAL; d = rcu_lock_domain_by_any_id(a.domid); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 00a9e70b7c..2b869ac997 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4104,6 +4104,7 @@ static int hvm_allow_set_param(struct domain *d, case HVM_PARAM_X87_FIP_WIDTH: break; /* The following parameters are deprecated. */ + case HVM_PARAM_PAE_ENABLED: case HVM_PARAM_DM_DOMAIN: case HVM_PARAM_BUFIOREQ_EVTCHN: rc = -EPERM; @@ -4410,6 +4411,7 @@ static int hvm_allow_get_param(struct domain *d, case HVM_PARAM_X87_FIP_WIDTH: break; /* The following parameters are deprecated. */ + case HVM_PARAM_PAE_ENABLED: case HVM_PARAM_DM_DOMAIN: case HVM_PARAM_BUFIOREQ_EVTCHN: rc = -ENODATA; diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index 36832e4b94..faa6bda095 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -86,7 +86,7 @@ #define HVM_PARAM_STORE_PFN 1 #define HVM_PARAM_STORE_EVTCHN 2 -#define HVM_PARAM_PAE_ENABLED 4 +#define HVM_PARAM_PAE_ENABLED 4 /* Obsolete. Do not use. */ #define HVM_PARAM_IOREQ_PFN 5
HVM_PARAM_PAE_ENABLED is undocumented and Xen has never acted upon its value, contrary perhaps to expectations based on how other boolean fields work. It was only ever used as a non-standard calling convention for xc_cpuid_apply_policy() but that has been fixed now. Purge its use, and any possible confusion over its behaviour, by having Xen reject any attempts to use it. Forgo setting it up in libxl's hvm_set_conf_params(). The only backwards compatibility necessary is to have the HVM restore stream discard it if found. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> --- tools/libxc/xc_sr_restore_x86_hvm.c | 10 ++++++++++ tools/libxc/xc_sr_save_x86_hvm.c | 1 - tools/libxl/libxl_x86.c | 8 +------- xen/arch/arm/hvm.c | 3 ++- xen/arch/x86/hvm/hvm.c | 2 ++ xen/include/public/hvm/params.h | 2 +- 6 files changed, 16 insertions(+), 10 deletions(-)