| Message ID | 20200127143444.25538-17-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support CPUID/MSR data in migration streams | expand |
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
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
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; }
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(-)