[v2,16/17] tools/libxc: Restore CPUID/MSR data found in the migration stream
diff mbox series

Message ID 20200127143444.25538-17-andrew.cooper3@citrix.com
State New
Headers show
Series
  • Support CPUID/MSR data in migration streams
Related show

Commit Message

Andrew Cooper Jan. 27, 2020, 2:34 p.m. UTC
With all other pieces in place, it is now safe to restore the CPUID and MSR
data in the migration stream, rather than discarding them and using the higher
level toolstacks compatibility logic.

While this is a small patch, it has large implications for migrated/resumed
domains.  Most obviously, the CPU family/model/stepping data,
cache/tlb/etc. will no longer change behind the guests back.

Another change is the interpretation of the Xend cpuid strings.  The 'k'
option is not a sensible thing to have ever supported, and 's' is how how the
stream will end up behaving.

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: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 tools/libxc/xc_cpuid_x86.c     |  8 ++++----
 tools/libxc/xc_sr_common_x86.c | 26 ++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 6 deletions(-)

Comments

Jan Beulich Jan. 27, 2020, 2:51 p.m. UTC | #1
On 27.01.2020 15:34, Andrew Cooper wrote:
> With all other pieces in place, it is now safe to restore the CPUID and MSR
> data in the migration stream, rather than discarding them and using the higher
> level toolstacks compatibility logic.
> 
> While this is a small patch, it has large implications for migrated/resumed
> domains.  Most obviously, the CPU family/model/stepping data,
> cache/tlb/etc. will no longer change behind the guests back.
> 
> Another change is the interpretation of the Xend cpuid strings.  The 'k'
> option is not a sensible thing to have ever supported, and 's' is how how the
> stream will end up behaving.

I'm a little confused I guess - I'd have expected such a description
to mean that ...

> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -291,10 +291,9 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
>   *   '0' -> force to 0
>   *   'x' -> we don't care (use default)
>   *   'k' -> pass through host value
> - *   's' -> pass through the first time and then keep the same value
> - *          across save/restore and migration.
> + *   's' -> legacy alias for 'k'

... 's' remains documented as is, and 'k' to become a legacy alias.

Jan
Andrew Cooper Jan. 27, 2020, 3:52 p.m. UTC | #2
On 27/01/2020 14:51, Jan Beulich wrote:
> On 27.01.2020 15:34, Andrew Cooper wrote:
>> With all other pieces in place, it is now safe to restore the CPUID and MSR
>> data in the migration stream, rather than discarding them and using the higher
>> level toolstacks compatibility logic.
>>
>> While this is a small patch, it has large implications for migrated/resumed
>> domains.  Most obviously, the CPU family/model/stepping data,
>> cache/tlb/etc. will no longer change behind the guests back.
>>
>> Another change is the interpretation of the Xend cpuid strings.  The 'k'
>> option is not a sensible thing to have ever supported, and 's' is how how the
>> stream will end up behaving.
> I'm a little confused I guess - I'd have expected such a description
> to mean that ...
>
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -291,10 +291,9 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
>>   *   '0' -> force to 0
>>   *   'x' -> we don't care (use default)
>>   *   'k' -> pass through host value
>> - *   's' -> pass through the first time and then keep the same value
>> - *          across save/restore and migration.
>> + *   's' -> legacy alias for 'k'
> ... 's' remains documented as is, and 'k' to become a legacy alias.

Given that s has never worked in the past, k is the only plausible one
used by people.

As they mean the same now, we could specify it either way around, but
this way round gives users less work to do.

~Andrew

Patch
diff mbox series

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index ac38c1406e..c78d00bbc3 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -291,10 +291,9 @@  int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
  *   '0' -> force to 0
  *   'x' -> we don't care (use default)
  *   'k' -> pass through host value
- *   's' -> pass through the first time and then keep the same value
- *          across save/restore and migration.
+ *   's' -> legacy alias for 'k'
  *
- * For 's' and 'x' the configuration is overwritten with the value applied.
+ * In all cases, the returned string consists of just '0' and '1'.
  */
 int xc_cpuid_set(
     xc_interface *xch, uint32_t domid, const unsigned int *input,
@@ -420,7 +419,8 @@  int xc_cpuid_set(
                 clear_feature(31 - j, regs[i]);
 
             config_transformed[i][j] = config[i][j];
-            if ( config[i][j] == 's' )
+            /* All non 0/1 values get overwritten. */
+            if ( (config[i][j] & ~1) != '0' )
                 config_transformed[i][j] = '0' + val;
         }
     }
diff --git a/tools/libxc/xc_sr_common_x86.c b/tools/libxc/xc_sr_common_x86.c
index a849891634..77ea044a74 100644
--- a/tools/libxc/xc_sr_common_x86.c
+++ b/tools/libxc/xc_sr_common_x86.c
@@ -134,8 +134,30 @@  int handle_x86_msr_policy(struct xc_sr_context *ctx, struct xc_sr_record *rec)
 
 int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
 {
-    /* TODO: Become conditional on there being no data in the stream. */
-    *missing = XGR_SDD_MISSING_MSR | XGR_SDD_MISSING_CPUID;
+    xc_interface *xch = ctx->xch;
+    uint32_t nr_leaves = 0, nr_msrs = 0;
+    uint32_t err_l = ~0, err_s = ~0, err_m = ~0;
+
+    if ( ctx->x86.restore.cpuid.ptr )
+        nr_leaves = ctx->x86.restore.cpuid.size / sizeof(xen_cpuid_leaf_t);
+    else
+        *missing |= XGR_SDD_MISSING_CPUID;
+
+    if ( ctx->x86.restore.msr.ptr )
+        nr_msrs = ctx->x86.restore.msr.size / sizeof(xen_msr_entry_t);
+    else
+        *missing |= XGR_SDD_MISSING_MSR;
+
+    if ( (nr_leaves || nr_msrs) &&
+         xc_set_domain_cpu_policy(xch, ctx->domid,
+                                  nr_leaves, ctx->x86.restore.cpuid.ptr,
+                                  nr_msrs,   ctx->x86.restore.msr.ptr,
+                                  &err_l, &err_s, &err_m) )
+    {
+        PERROR("Failed to set CPUID policy: leaf %08x, subleaf %08x, msr %08x",
+               err_l, err_s, err_m);
+        return -1;
+    }
 
     return 0;
 }