| Message ID | 20200127143444.25538-11-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 10/17] tools/libxl: Plumb a restore boolean down into libxl__build_pre()"):
> To fix CPUID handling, libxl__build_pre() is going to have to distinguish
> between a brand new VM vs one which is being migrated-in/resumed.
The distinction between libxl__build_pre and these other functions is
not particularly principled. It is a legacy from an old API (prior to
the existince of *create) where libxl callers were expected to "build"
a domain first and then do other things to it.
Maybe it would be better to pass this via libxl__domain_build_state
rather than as an additional parameter ?
Ian.
On 24/02/2020 17:39, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH v2 10/17] tools/libxl: Plumb a restore boolean down into libxl__build_pre()"): >> To fix CPUID handling, libxl__build_pre() is going to have to distinguish >> between a brand new VM vs one which is being migrated-in/resumed. > The distinction between libxl__build_pre and these other functions is > not particularly principled. It is a legacy from an old API (prior to > the existince of *create) where libxl callers were expected to "build" > a domain first and then do other things to it. > > Maybe it would be better to pass this via libxl__domain_build_state > rather than as an additional parameter ? Well - I tried a similar approach the first time around, and it broke stubdoms so badly it needed reverting. (Untrim the commit details) > v2: > * New. This is c/s aacc1430064 "tools/libxl: Plumb domain_create_state down > into libxl__build_pre()" take-2, without any collateral damage to stubdoms. The actual information we want is in libxl__domain_create_state (specifically, restore_fd >= -1). I first tried plumbing dcs down, to avoid stashing the same information in two different structures at different times. Sadly, plumbing dcs didn't work because it is common between the real domain and the stubdom (and this lead to the stubdom getting no settings at all). What we want to do is only influence the CPUID construction of the main domain (which may be migrating in), whereas the stubdom always wants fresh settings. I could duplicate it into dbs, and at a guess that would probably work, but isn't it taking a bad problem and making it worse? ~Andrew
Andrew Cooper writes ("Re: [PATCH v2 10/17] tools/libxl: Plumb a restore boolean down into libxl__build_pre()"):
> On 24/02/2020 17:39, Ian Jackson wrote:
> > Maybe it would be better to pass this via libxl__domain_build_state
> > rather than as an additional parameter ?
>
> Well - I tried a similar approach the first time around, and it broke
> stubdoms so badly it needed reverting.
>
> (Untrim the commit details)
>
> > v2:
> > * New. This is c/s aacc1430064 "tools/libxl: Plumb domain_create_state down
> > into libxl__build_pre()" take-2, without any collateral damage to stubdoms.
>
> The actual information we want is in libxl__domain_create_state
> (specifically, restore_fd >= -1). I first tried plumbing dcs down, to
> avoid stashing the same information in two different structures at
> different times.
>
> Sadly, plumbing dcs didn't work because it is common between the real
> domain and the stubdom (and this lead to the stubdom getting no settings
> at all). What we want to do is only influence the CPUID construction of
> the main domain (which may be migrating in), whereas the stubdom always
> wants fresh settings.
Right. Thanks for the explanation, which make sense to me.
> I could duplicate it into dbs, and at a guess that would probably work,
> but isn't it taking a bad problem and making it worse?
I think that is fine.
Conceptually I think it's like this:
These structs take place of huge lists of parameters. When we create
a service domain, we need to pass a new list of parameters
(_build_state), we also pass some of the original ones
(_create_state). If we are talking about parameters that need to be
different for a service domain, they should be in _build_state; even
if the main domain's version is effectively a copy of something in
_create_state.
So I think adding a "restoring" or "restore" boolean to dbs is
probably right.
Thanks,
Ian.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 12113185ac..1d2e193509 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -434,14 +434,15 @@ static void init_console_info(libxl__gc *gc, int libxl__domain_build(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid, - libxl__domain_build_state *state) + libxl__domain_build_state *state, + bool restore) { libxl_domain_build_info *const info = &d_config->b_info; char **vments = NULL, **localents = NULL; struct timeval start_time; int i, ret; - ret = libxl__build_pre(gc, domid, d_config, state); + ret = libxl__build_pre(gc, domid, d_config, state, restore); if (ret) goto out; @@ -1218,7 +1219,7 @@ static void domcreate_bootloader_done(libxl__egc *egc, dcs->sdss.callback = domcreate_devmodel_started; if (restore_fd < 0 && dcs->domid_soft_reset == INVALID_DOMID) { - rc = libxl__domain_build(gc, d_config, domid, state); + rc = libxl__domain_build(gc, d_config, domid, state, false); domcreate_rebuild_done(egc, dcs, rc); return; } @@ -1245,7 +1246,7 @@ static void domcreate_bootloader_done(libxl__egc *egc, goto out; } - rc = libxl__build_pre(gc, domid, d_config, state); + rc = libxl__build_pre(gc, domid, d_config, state, restore_fd >= 0); if (rc) goto out; diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index e92e412c1b..d3dfa8751c 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -2197,7 +2197,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss) if (ret) goto out; uint32_t dm_domid = sdss->pvqemu.guest_domid; - ret = libxl__domain_build(gc, dm_config, dm_domid, stubdom_state); + ret = libxl__domain_build(gc, dm_config, dm_domid, stubdom_state, false); if (ret) goto out; diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index b730365f47..1bac277351 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -244,7 +244,9 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid, } int libxl__build_pre(libxl__gc *gc, uint32_t domid, - libxl_domain_config *d_config, libxl__domain_build_state *state) + libxl_domain_config *d_config, + libxl__domain_build_state *state, + bool restore) { libxl_domain_build_info *const info = &d_config->b_info; libxl_ctx *ctx = libxl__gc_owner(gc); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 50856ca703..e66b068d16 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1390,7 +1390,8 @@ _hidden void libxl__domain_build_state_dispose(libxl__domain_build_state *s); _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid, libxl_domain_config * const d_config, - libxl__domain_build_state *state); + libxl__domain_build_state *state, + bool restore); _hidden int libxl__build_post(libxl__gc *gc, uint32_t domid, libxl_domain_build_info *info, libxl__domain_build_state *state, char **vms_ents, char **local_ents); @@ -1963,7 +1964,8 @@ _hidden int libxl__domain_make(libxl__gc *gc, _hidden int libxl__domain_build(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid, - libxl__domain_build_state *state); + libxl__domain_build_state *state, + bool restore); /* for device model creation */ _hidden const char *libxl__domain_device_model(libxl__gc *gc,
To fix CPUID handling, libxl__build_pre() is going to have to distinguish between a brand new VM vs one which is being migrated-in/resumed. No functional 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> v2: * New. This is c/s aacc1430064 "tools/libxl: Plumb domain_create_state down into libxl__build_pre()" take-2, without any collateral damage to stubdoms. --- tools/libxl/libxl_create.c | 9 +++++---- tools/libxl/libxl_dm.c | 2 +- tools/libxl/libxl_dom.c | 4 +++- tools/libxl/libxl_internal.h | 6 ++++-- 4 files changed, 13 insertions(+), 8 deletions(-)