| Message ID | 20200103130616.13724-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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.
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
Andrew Cooper writes ("Re: [PATCH 17/20] tools/libx[cl]: Plumb static_data_done() up into libxl"):
> 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.
Thanks for the explanation.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
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
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(-)