[19/20] tools/libxl: Re-position CPUID handling during domain construction

Commit Message

Andrew Cooper Jan. 3, 2020, 1:06 p.m. UTC

CPUID handling needs to be earlier in construction.  Move it from its current
position in libxl__build_post() to libxl__build_pre() for fresh builds, and
libxl__srm_callout_callback_static_data_done() for the migration/resume case.

In the migration case, take account of XGR_SDD_MISSING_CPUID.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Ian Jackson <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Anthony PERARD <[e-mail]>
---
 tools/libxl/libxl_create.c | 19 ++++++++++++++++++-
 tools/libxl/libxl_dom.c    | 12 ++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

Comments

Ian Jackson Jan. 14, 2020, 5:33 p.m. UTC | #1

Andrew Cooper writes ("[PATCH 19/20] tools/libxl: Re-position CPUID handling during domain construction"):
> CPUID handling needs to be earlier in construction.  Move it from its current
> position in libxl__build_post() to libxl__build_pre() for fresh builds, and
> libxl__srm_callout_callback_static_data_done() for the migration/resume case.
> 
> In the migration case, take account of XGR_SDD_MISSING_CPUID.

Is it possible to split out the change to the sequencing, from the new
functionality ?  If so, please could you do so.

Thanks,
Ian.

Andrew Cooper Jan. 14, 2020, 5:51 p.m. UTC | #2

On 14/01/2020 17:33, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 19/20] tools/libxl: Re-position CPUID handling during domain construction"):
>> CPUID handling needs to be earlier in construction.  Move it from its current
>> position in libxl__build_post() to libxl__build_pre() for fresh builds, and
>> libxl__srm_callout_callback_static_data_done() for the migration/resume case.
>>
>> In the migration case, take account of XGR_SDD_MISSING_CPUID.
> Is it possible to split out the change to the sequencing, from the new
> functionality ?

Not easily, no.

While libxc is discarding the CPUID record, libxl needs to make the
calls unconditionally for VMs to function.

If you recall, an early patch in the series leaves a todo in libxc
(which is there for this very bisection reason), which passes
XGR_SDD_MISSING_CPUID back unconditionally (making this patch alone
effectively no net change).

A subsequence patch makes the passing of XGR_SDD_MISSING_CPUID
conditional on the content in the stream, now that we are no longer
discarding the CPUID records found.

~Andrew

Ian Jackson Jan. 14, 2020, 6:12 p.m. UTC | #3

Andrew Cooper writes ("Re: [PATCH 19/20] tools/libxl: Re-position CPUID handling during domain construction"):
> On 14/01/2020 17:33, Ian Jackson wrote:
> > Is it possible to split out the change to the sequencing, from the new
> > functionality ?
> 
> Not easily, no.

Thanks for the explanation.  I read it again with that in mind.

Acked-by: Ian Jackson <[e-mail]>

Patch

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a8fa4ca11b..0d9a91aeeb 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1303,8 +1303,25 @@  int libxl__srm_callout_callback_static_data_done(unsigned int missing,
     libxl__save_helper_state *shs = user;
     libxl__domain_create_state *dcs = shs->caller_state;
     STATE_AO_GC(dcs->ao);
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    const libxl_domain_config *d_config = dcs->guest_config;
+    const libxl_domain_build_info *info = &d_config->b_info;
 
-    /* TODO - something useful. */
+    /*
+     * For pre-4.14 streams, no CPUID/MSR information will be sent.  Retain
+     * the old behaviour by regenerating CPUID from first principles.
+     */
+    if (missing & XGR_SDD_MISSING_CPUID)
+        libxl_cpuid_apply_policy(ctx, dcs->guest_domid);
+
+    /*
+     * In all cases, cpuid=[] needs re-evaluating.  The common case is that it
+     * will match libxl_cpuid_apply_policy() and/or whatever is in the stream,
+     * but the legacy XEND 'k' modifier passes through host values.
+     */
+    if (info->cpuid != NULL)
+        libxl_cpuid_set(ctx, dcs->guest_domid, info->cpuid);
 
     return 0;
 }
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 01aefa6ce4..099a913019 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -254,6 +254,8 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     int rc;
     uint64_t size;
 
+    const int restore_fd = dcs->restore_fd;
+
     if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
         LOG(ERROR, "Couldn't set max vcpu count");
         return ERROR_FAIL;
@@ -376,6 +378,12 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
         return ERROR_FAIL;
     }
 
+    if (restore_fd < 0) {
+        libxl_cpuid_apply_policy(ctx, domid);
+        if (info->cpuid != NULL)
+            libxl_cpuid_set(ctx, domid, info->cpuid);
+    }
+
     xs_domid = xs_read(ctx->xsh, XBT_NULL, "/tool/xenstored/domid", NULL);
     state->store_domid = xs_domid ? atoi(xs_domid) : 0;
     free(xs_domid);
@@ -456,10 +464,6 @@  int libxl__build_post(libxl__gc *gc, uint32_t domid,
     if (rc)
         return rc;
 
-    libxl_cpuid_apply_policy(ctx, domid);
-    if (info->cpuid != NULL)
-        libxl_cpuid_set(ctx, domid, info->cpuid);
-
     if (info->type == LIBXL_DOMAIN_TYPE_HVM
         && !libxl_ms_vm_genid_is_zero(&info->u.hvm.ms_vm_genid)) {
         rc = libxl__ms_vm_genid_set(gc, domid,