Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Ian Jackson" <Ian.Jackson@citrix.com>,
	"Anthony PERARD" <anthony.perard@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED
Date: Thu, 6 Feb 2020 11:32:39 +0000	[thread overview]
Message-ID: <5227980e-5938-f23b-7b1d-eac67bba792e@citrix.com> (raw)
In-Reply-To: <9c7f9930-02d7-13d3-88ab-bad236993e31@suse.com>

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

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