[6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED
diff mbox series

Message ID 20200205165056.11734-7-andrew.cooper3@citrix.com
State New
Headers show
Series
  • tools: Rationalise legacy CPUID handling
Related show

Commit Message

Andrew Cooper Feb. 5, 2020, 4:50 p.m. UTC
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(-)

Comments

Jan Beulich Feb. 6, 2020, 9:28 a.m. UTC | #1
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
Andrew Cooper Feb. 6, 2020, 11:32 a.m. UTC | #2
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
Jan Beulich Feb. 6, 2020, 11:37 a.m. UTC | #3
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
Andrew Cooper Feb. 6, 2020, 12:25 p.m. UTC | #4
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
Julien Grall Feb. 8, 2020, 12:12 p.m. UTC | #5
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>
Andrew Cooper Feb. 8, 2020, 12:15 p.m. UTC | #6
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
Ian Jackson Feb. 11, 2020, 5:49 p.m. UTC | #7
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.
Andrew Cooper Feb. 11, 2020, 6:03 p.m. UTC | #8
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

Patch
diff mbox series

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