[17/20] tools/libx[cl]: Plumb static_data_done() up into libxl
diff mbox series

Message ID 20200103130616.13724-4-andrew.cooper3@citrix.com
State New
Headers show
Series
  • Untitled series #223377
Related show

Commit Message

Andrew Cooper Jan. 3, 2020, 1:06 p.m. UTC
libxl is going to have to provide compatibility for pre 4.14 streams which
don't contain CPUID information.  Introduce the static_data_done() callback
and plumb it up into libxl.

No overall change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/include/xenguest.h     |  9 +++++++++
 tools/libxc/xc_sr_common_x86.c     | 13 ++++++++++++-
 tools/libxc/xc_sr_restore.c        |  1 +
 tools/libxl/libxl_create.c         | 13 +++++++++++++
 tools/libxl/libxl_save_msgs_gen.pl |  1 +
 5 files changed, 36 insertions(+), 1 deletion(-)

Comments

Ian Jackson Jan. 14, 2020, 5:30 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 17/20] tools/libx[cl]: Plumb static_data_done() up into libxl"):
>  /* callbacks provided by xc_domain_restore */
>  struct restore_callbacks {
> +    /*
> +     * Called once the STATIC_DATA_END record has been received/inferred.
> +     * Passes in the blocks of static data which have not been received, which
> +     * the higher level toolstack must provide backwards compatibility for.
> +     */
> +#define XGR_SDD_MISSING_CPUID (1 << 0)
> +#define XGR_SDD_MISSING_MSR   (1 << 1)
> +    int (*static_data_done)(unsigned int missing, void *data);

This is a bit weird, isn't it ?  I mean: if these blocks of data *are*
received then libxc handles them; but if they are not, libxc's caller
must do so.

I appreciate that the interface at the top of libxc is already rather
complex and uneven but this doesn't seem to be helping...

The actual code looks OK to me.

Thanks,
Ian.
Andrew Cooper Jan. 15, 2020, 4:34 p.m. UTC | #2
On 14/01/2020 17:30, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 17/20] tools/libx[cl]: Plumb static_data_done() up into libxl"):
>>  /* callbacks provided by xc_domain_restore */
>>  struct restore_callbacks {
>> +    /*
>> +     * Called once the STATIC_DATA_END record has been received/inferred.
>> +     * Passes in the blocks of static data which have not been received, which
>> +     * the higher level toolstack must provide backwards compatibility for.
>> +     */
>> +#define XGR_SDD_MISSING_CPUID (1 << 0)
>> +#define XGR_SDD_MISSING_MSR   (1 << 1)
>> +    int (*static_data_done)(unsigned int missing, void *data);
> This is a bit weird, isn't it ?  I mean: if these blocks of data *are*
> received then libxc handles them; but if they are not, libxc's caller
> must do so.
>
> I appreciate that the interface at the top of libxc is already rather
> complex and uneven but this doesn't seem to be helping...

There are several things going on here.

One is the control flow marker of "We reached the end of the static
data".  A higher level toolstack needs to know this unconditionally,
which is why the callback is mandatory.

For v2 compatibility, its callers cope with "this is where an end of
static data would be in a v3 stream", but that abstracted away so the
higher level toolstack doesn't know or need to care.

The missing parameter is "p.s. here are the things we were expecting but
didn't get - you need to pick up the pieces".  For now, it is synonymous
with "here is a v2 stream without CPUID data", but that won't be
accurate in the future if/when new static data records get retrofitted.

~Andrew

Patch
diff mbox series

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index d81baa6cc2..be80544bd0 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -140,6 +140,15 @@  int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom,
 
 /* callbacks provided by xc_domain_restore */
 struct restore_callbacks {
+    /*
+     * Called once the STATIC_DATA_END record has been received/inferred.
+     * Passes in the blocks of static data which have not been received, which
+     * the higher level toolstack must provide backwards compatibility for.
+     */
+#define XGR_SDD_MISSING_CPUID (1 << 0)
+#define XGR_SDD_MISSING_MSR   (1 << 1)
+    int (*static_data_done)(unsigned int missing, void *data);
+
     /* Called after a new checkpoint to suspend the guest. */
     int (*suspend)(void *data);
 
diff --git a/tools/libxc/xc_sr_common_x86.c b/tools/libxc/xc_sr_common_x86.c
index 09fb1d93d6..c3d1d30d91 100644
--- a/tools/libxc/xc_sr_common_x86.c
+++ b/tools/libxc/xc_sr_common_x86.c
@@ -44,9 +44,20 @@  int handle_x86_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec)
 
 int x86_static_data_complete(struct xc_sr_context *ctx)
 {
+    xc_interface *xch = ctx->xch;
+    unsigned int missing = 0;
+    int rc;
+
     /* TODO - something useful. */
+    missing = XGR_SDD_MISSING_MSR | XGR_SDD_MISSING_CPUID;
 
-    return 0;
+    rc = ctx->restore.callbacks->static_data_done(
+        missing, ctx->restore.callbacks->data);
+
+    if ( rc )
+        ERROR("static_data_done() callback failed: %d\n", rc);
+
+    return rc;
 }
 
 int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index d4bd60a31e..09d8a08316 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -908,6 +908,7 @@  int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         assert(callbacks->checkpoint);
         /* Fallthrough */
     case XC_STREAM_PLAIN:
+        assert(callbacks->static_data_done);
         break;
 
     default:
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 32d45dcef0..fdc76917dc 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1227,6 +1227,7 @@  static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->srs.dcs = dcs;
 
     /* Restore */
+    callbacks->static_data_done = libxl__srm_callout_callback_static_data_done;
     callbacks->restore_results = libxl__srm_callout_callback_restore_results;
 
     /* COLO only supports HVM now because it does not work very
@@ -1296,6 +1297,18 @@  static void libxl__colo_restore_setup_done(libxl__egc *egc,
     libxl__stream_read_start(egc, &dcs->srs);
 }
 
+int libxl__srm_callout_callback_static_data_done(unsigned int missing,
+                                                 void *user)
+{
+    libxl__save_helper_state *shs = user;
+    libxl__domain_create_state *dcs = shs->caller_state;
+    STATE_AO_GC(dcs->ao);
+
+    /* TODO - something useful. */
+
+    return 0;
+}
+
 void libxl__srm_callout_callback_restore_results(xen_pfn_t store_mfn,
           xen_pfn_t console_mfn, void *user)
 {
diff --git a/tools/libxl/libxl_save_msgs_gen.pl b/tools/libxl/libxl_save_msgs_gen.pl
index 831a15e0bb..5bfbd4fd10 100755
--- a/tools/libxl/libxl_save_msgs_gen.pl
+++ b/tools/libxl/libxl_save_msgs_gen.pl
@@ -29,6 +29,7 @@  our @msgs = (
     [ 'srcxA',  "wait_checkpoint", [] ],
     [ 'scxA',   "switch_qemu_logdirty",  [qw(uint32_t domid
                                           unsigned enable)] ],
+    [ 'rcxW',   "static_data_done",      [qw(unsigned missing)] ],
     [ 'rcx',    "restore_results",       ['xen_pfn_t', 'store_gfn',
                                           'xen_pfn_t', 'console_gfn'] ],
     [ 'srW',    "complete",              [qw(int retval