| Message ID | 20200127143444.25538-8-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support CPUID/MSR data in migration streams | expand |
Andrew Cooper writes ("[PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"):
> A v3 stream can compatibly read a v2 stream by inferring the position of the
> STATIC_DATA_END record.
>
> v2 compatibility is only needed for x86. No other architectures exist yet,
> but they will have a minimum of v3 when introduced.
>
> The x86 HVM compatibility point being in handle_page_data() (which is common
> code) is a bit awkward. However, as the two compatibility points are subtly
> different, and it is (intentionally) not possible to call into arch specific
> code from common code (except via the ops hooks), use some #ifdef-ary and
> opencode the check, rather than make handle_page_data() a per-arch helper.
...
> +#if defined(__i386__) || defined(__x86_64__)
> + /* v2 compat. Infer the position of STATIC_DATA_END. */
> + if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end )
> + {
> + rc = handle_static_data_end(ctx);
> + if ( rc )
These 17 lines appears twice, in basically identical form. Could it
be refactored ?
Thanks,
Ian.
On 24/02/2020 17:32, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"): >> A v3 stream can compatibly read a v2 stream by inferring the position of the >> STATIC_DATA_END record. >> >> v2 compatibility is only needed for x86. No other architectures exist yet, >> but they will have a minimum of v3 when introduced. >> >> The x86 HVM compatibility point being in handle_page_data() (which is common >> code) is a bit awkward. However, as the two compatibility points are subtly >> different, and it is (intentionally) not possible to call into arch specific >> code from common code (except via the ops hooks), use some #ifdef-ary and >> opencode the check, rather than make handle_page_data() a per-arch helper. > ... >> +#if defined(__i386__) || defined(__x86_64__) >> + /* v2 compat. Infer the position of STATIC_DATA_END. */ >> + if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end ) >> + { >> + rc = handle_static_data_end(ctx); >> + if ( rc ) > These 17 lines appears twice, in basically identical form. Could it > be refactored ? Not really, no. The error handling (i.e. half of those 17 lines) is different, making it somewhat awkward to fit into a static inline. More importantly however, by design, common code can't call arch-specific code without a restore_ops hook. Deduping these would require breaking the restriction which is currently doing a decent job of keeping x86-isms out of common code. ~Andrew
Andrew Cooper writes ("Re: [PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"):
> On 24/02/2020 17:32, Ian Jackson wrote:
> > These 17 lines appears twice, in basically identical form. Could it
> > be refactored ?
>
> Not really, no.
>
> The error handling (i.e. half of those 17 lines) is different, making it
> somewhat awkward to fit into a static inline.
You could handle that with a small bit of code around one of the call
sites to adjust the error handling. (Also, what a mess, but I guess
we're here now...)
> More importantly however, by design, common code can't call
> arch-specific code without a restore_ops hook. Deduping these would
> require breaking the restriction which is currently doing a decent job
> of keeping x86-isms out of common code.
I'm afraid you're going to have to explain that to me at a bit greater
length. The biggest thing that is confusing me about your statement
here is that your patch is indeed adding x86-specific code to a common
file. But also I don't understand the comment about restore_ops
hooks - do you mean something in restore_callbacks ?
Thanks,
Ian.
On 05/03/2020 16:24, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"): >> On 24/02/2020 17:32, Ian Jackson wrote: >>> These 17 lines appears twice, in basically identical form. Could it >>> be refactored ? >> Not really, no. >> >> The error handling (i.e. half of those 17 lines) is different, making it >> somewhat awkward to fit into a static inline. > You could handle that with a small bit of code around one of the call > sites to adjust the error handling. (Also, what a mess, but I guess > we're here now...) ... which is the majority the code you're trying to abstract away. > >> More importantly however, by design, common code can't call >> arch-specific code without a restore_ops hook. Deduping these would >> require breaking the restriction which is currently doing a decent job >> of keeping x86-isms out of common code. > I'm afraid you're going to have to explain that to me at a bit greater > length. The biggest thing that is confusing me about your statement > here is that your patch is indeed adding x86-specific code to a common > file. But also I don't understand the comment about restore_ops > hooks - do you mean something in restore_callbacks ? No. restore_callbacks are plumbing between libxl-save-helper and libxc. restore_ops are internal to the restore code, and come in x86_pv and x86_hvm flavours, with ARM existing in some theoretical future. The design of the common save/restore code had deliberate measures put in place to make it hard to get arch-specific details slip into common code, so porting to different architectures didn't have to start by doing a bunch of cleanup. tl;dr I could put an #ifdef x86'd static inline in the root common header (xc_sr_common.h), but the overall complexity is greater than what is presented here. ~Andrew
Andrew Cooper writes ("Re: [PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"):
> On 05/03/2020 16:24, Ian Jackson wrote:
> > Andrew Cooper writes ("Re: [PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"):
> >> More importantly however, by design, common code can't call
> >> arch-specific code without a restore_ops hook. Deduping these would
> >> require breaking the restriction which is currently doing a decent job
> >> of keeping x86-isms out of common code.
> > I'm afraid you're going to have to explain that to me at a bit greater
> > length. The biggest thing that is confusing me about your statement
> > here is that your patch is indeed adding x86-specific code to a common
> > file. But also I don't understand the comment about restore_ops
> > hooks - do you mean something in restore_callbacks ?
>
> No. restore_callbacks are plumbing between libxl-save-helper and libxc.
>
> restore_ops are internal to the restore code, and come in x86_pv and
> x86_hvm flavours, with ARM existing in some theoretical future. The
> design of the common save/restore code had deliberate measures put in
> place to make it hard to get arch-specific details slip into common
> code, so porting to different architectures didn't have to start by
> doing a bunch of cleanup.
>
> tl;dr I could put an #ifdef x86'd static inline in the root common
> header (xc_sr_common.h), but the overall complexity is greater than what
> is presented here.
Well, I still don't quite follow but as you point out on irc I haven't
replied for too long. I don't think I should withhold my ack at this
stage.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h index efd90b0d42..b4df8d0ffe 100644 --- a/tools/libxc/include/xenguest.h +++ b/tools/libxc/include/xenguest.h @@ -139,7 +139,7 @@ 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. */ + /* Called once the STATIC_DATA_END record has been received/inferred. */ int (*static_data_done)(void *data); /* Called after a new checkpoint to suspend the guest. */ diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h index ae0ab70f76..51e3d3ee3b 100644 --- a/tools/libxc/xc_sr_common.h +++ b/tools/libxc/xc_sr_common.h @@ -253,7 +253,7 @@ struct xc_sr_context /* Currently buffering records between a checkpoint */ bool buffer_all_records; - /* Whether a STATIC_DATA_END record has been seen. */ + /* Whether a STATIC_DATA_END record has been seen/inferred. */ bool seen_static_data_end; /* @@ -428,6 +428,9 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec); int populate_pfns(struct xc_sr_context *ctx, unsigned int count, const xen_pfn_t *original_pfns, const uint32_t *types); +/* Handle a STATIC_DATA_END record. */ +int handle_static_data_end(struct xc_sr_context *ctx); + #endif /* * Local variables: diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c index 9c924387ae..bb94cd879d 100644 --- a/tools/libxc/xc_sr_restore.c +++ b/tools/libxc/xc_sr_restore.c @@ -342,6 +342,31 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec) xen_pfn_t *pfns = NULL, pfn; uint32_t *types = NULL, type; + /* + * v2 compatibility only exists for x86 streams. This is a bit of a + * bodge, but it is less bad than duplicating handle_page_data() between + * different architectures. + */ +#if defined(__i386__) || defined(__x86_64__) + /* v2 compat. Infer the position of STATIC_DATA_END. */ + if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end ) + { + rc = handle_static_data_end(ctx); + if ( rc ) + { + ERROR("Inferred STATIC_DATA_END record failed"); + goto err; + } + rc = -1; + } + + if ( !ctx->restore.seen_static_data_end ) + { + ERROR("No STATIC_DATA_END seen"); + goto err; + } +#endif + if ( rec->length < sizeof(*pages) ) { ERROR("PAGE_DATA record truncated: length %u, min %zu", @@ -631,7 +656,7 @@ static int buffer_record(struct xc_sr_context *ctx, struct xc_sr_record *rec) return 0; } -static int handle_static_data_end(struct xc_sr_context *ctx) +int handle_static_data_end(struct xc_sr_context *ctx) { xc_interface *xch = ctx->xch; int rc = 0; diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c index 16e738884e..3756225be6 100644 --- a/tools/libxc/xc_sr_restore_x86_pv.c +++ b/tools/libxc/xc_sr_restore_x86_pv.c @@ -679,6 +679,23 @@ static int handle_x86_pv_p2m_frames(struct xc_sr_context *ctx, unsigned int start, end, x, fpp = PAGE_SIZE / ctx->x86.pv.width; int rc; + /* v2 compat. Infer the position of STATIC_DATA_END. */ + if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end ) + { + rc = handle_static_data_end(ctx); + if ( rc ) + { + ERROR("Inferred STATIC_DATA_END record failed"); + return rc; + } + } + + if ( !ctx->restore.seen_static_data_end ) + { + ERROR("No STATIC_DATA_END seen"); + return -1; + } + if ( !ctx->x86.pv.restore.seen_pv_info ) { ERROR("Not yet received X86_PV_INFO record");
A v3 stream can compatibly read a v2 stream by inferring the position of the STATIC_DATA_END record. v2 compatibility is only needed for x86. No other architectures exist yet, but they will have a minimum of v3 when introduced. The x86 HVM compatibility point being in handle_page_data() (which is common code) is a bit awkward. However, as the two compatibility points are subtly different, and it is (intentionally) not possible to call into arch specific code from common code (except via the ops hooks), use some #ifdef-ary and opencode the check, rather than make handle_page_data() a per-arch helper. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Wei Liu <wl@xen.org> v2: * Split/rearranged from v1 * Rewrite the commit message to explain why compatibility is done this way. --- tools/libxc/include/xenguest.h | 2 +- tools/libxc/xc_sr_common.h | 5 ++++- tools/libxc/xc_sr_restore.c | 27 ++++++++++++++++++++++++++- tools/libxc/xc_sr_restore_x86_pv.c | 17 +++++++++++++++++ 4 files changed, 48 insertions(+), 3 deletions(-)