| Message ID | 20200527173407.1398-6-paul@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | domain context infrastructure | expand |
On 27/05/2020 18:34, Paul Durrant wrote: > ... in the save/restore code. > > This patch replaces direct mapping of the shared_info_frame (retrieved > using XEN_DOMCTL_getdomaininfo) with save/load of the domain context > SHARED_INFO record. > > No modifications are made to the definition of the migration stream at > this point. Subsequent patches will define a record in the libxc domain > image format for passing domain context and convert the save/restore code > to use that. > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> I was going to fix up the remaining issues and commit this, but there is a rather major problem in the way. Therefore, here is a rather more full review. > diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c > index dd9a11b4b5..1acb3765aa 100644 > --- a/tools/libxc/xc_sr_common.c > +++ b/tools/libxc/xc_sr_common.c > @@ -138,6 +138,73 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec) > return 0; > }; > > +int get_domain_context(struct xc_sr_context *ctx) > +{ > + xc_interface *xch = ctx->xch; > + size_t len = 0; > + int rc; > + > + if ( ctx->domain_context.buffer ) > + { > + ERROR("Domain context already present"); > + return -1; > + } > + > + rc = xc_domain_getcontext(xch, ctx->domid, NULL, &len); > + if ( rc < 0 ) > + { > + PERROR("Unable to get size of domain context"); > + return -1; > + } > + > + ctx->domain_context.buffer = malloc(len); > + if ( !ctx->domain_context.buffer ) > + { > + PERROR("Unable to allocate memory for domain context"); > + return -1; > + } There is an xc_sr_blob interface, as this is a common pattern. > + > + rc = xc_domain_getcontext(xch, ctx->domid, ctx->domain_context.buffer, > + &len); > + if ( rc < 0 ) > + { > + PERROR("Unable to get domain context"); > + return -1; > + } > + > + ctx->domain_context.len = len; > + > + return 0; > +} > + > +int set_domain_context(struct xc_sr_context *ctx) > +{ > + xc_interface *xch = ctx->xch; > + int rc; > + > + if ( !ctx->domain_context.buffer ) > + { > + ERROR("Domain context not present"); > + return -1; > + } > + > + rc = xc_domain_setcontext(xch, ctx->domid, ctx->domain_context.buffer, > + ctx->domain_context.len); > + > + if ( rc < 0 ) > + { > + PERROR("Unable to set domain context"); > + return -1; > + } > + > + return 0; > +} > + > +void common_cleanup(struct xc_sr_context *ctx) > +{ > + free(ctx->domain_context.buffer); There is only a single caller to this function, so there is a (possibly latent) memory leak. > +} > + > static void __attribute__((unused)) build_assertions(void) > { > BUILD_BUG_ON(sizeof(struct xc_sr_ihdr) != 24); > diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h > index 5dd51ccb15..0d61978b08 100644 > --- a/tools/libxc/xc_sr_common.h > +++ b/tools/libxc/xc_sr_common.h > @@ -208,6 +208,11 @@ struct xc_sr_context > > xc_dominfo_t dominfo; > > + struct { > + void *buffer; > + unsigned int len; > + } domain_context; As noted above, xc_sr_blob domain_context; > diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c > index 904ccc462a..21982a38ad 100644 > --- a/tools/libxc/xc_sr_restore_x86_pv.c > +++ b/tools/libxc/xc_sr_restore_x86_pv.c > @@ -865,7 +865,7 @@ static int handle_shared_info(struct xc_sr_context *ctx, > xc_interface *xch = ctx->xch; > unsigned int i; > int rc = -1; > - shared_info_any_t *guest_shinfo = NULL; > + shared_info_any_t *guest_shinfo; > const shared_info_any_t *old_shinfo = rec->data; > > if ( !ctx->x86.pv.restore.seen_pv_info ) > @@ -878,18 +878,14 @@ static int handle_shared_info(struct xc_sr_context *ctx, > { > ERROR("X86_PV_SHARED_INFO record wrong size: length %u" > ", expected 4096", rec->length); > - goto err; > + return -1; > } > > - guest_shinfo = xc_map_foreign_range( > - xch, ctx->domid, PAGE_SIZE, PROT_READ | PROT_WRITE, > - ctx->dominfo.shared_info_frame); > - if ( !guest_shinfo ) > - { > - PERROR("Failed to map Shared Info at mfn %#lx", > - ctx->dominfo.shared_info_frame); > - goto err; > - } > + rc = x86_pv_get_shinfo(ctx); > + if ( rc ) > + return rc; If I'm following this logic correctly, we're now in the final throws of completing migration with data in hand, and we ask the hypervisor to gather the entire domain context for this (not-yet-run) guest, copy it (twice, even) down into userspace, so we can scan through it to find the appropriate tag, copy less than a page's worth of data, then pass the full buffer back to Xen to unserialise the whole thing over the guest. The restore path shouldn't be calling DOMCTL_get* at all. It is conceptually wrong, and a waste of time/effort which would be better spent with the guest unpaused. What the restore path should be doing is passing data from the stream, straight into DOMCTL_set* (and attempting to do this will probably demonstrate why hvmctxt was an especially poor API to copy). The layout of existing migration-v2 blocks was designed around blobs which Xen produces and consumes directly, specifically to minimise the processing required. I think it is a layering violation to have libxc pick apart and reframe the internals of another layers' blob. What is not currently clear is whether the domain context wants sending as a discrete blob itself (and have a new chunk type allocated for the purpose), or each bit of it is going to want picking apart. This largely depends on what else is going in the blob at a Xen level. Also, I'd like to see the plans for the HVM side of this logic before deciding on whether the PV side is appropriate. I know we have a dedicated SHARED_INFO record right now, but it would be fine (AFAICT) to relax the migration spec to state that one of {SHARED_INFO, DOMAIN_CONTEXT} must be sent for PV. > @@ -854,13 +835,27 @@ static int write_x86_pv_p2m_frames(struct xc_sr_context *ctx) > */ > static int write_shared_info(struct xc_sr_context *ctx) > { > + xc_interface *xch = ctx->xch; > struct xc_sr_record rec = { > .type = REC_TYPE_SHARED_INFO, > .length = PAGE_SIZE, > - .data = ctx->x86.pv.shinfo, > }; > + int rc; > > - return write_record(ctx, &rec); > + if ( !(rec.data = calloc(1, PAGE_SIZE)) ) > + { > + ERROR("Cannot allocate buffer for SHARED_INFO data"); > + return -1; > + } > + > + BUILD_BUG_ON(sizeof(*ctx->x86.pv.shinfo) > PAGE_SIZE); > + memcpy(rec.data, ctx->x86.pv.shinfo, sizeof(*ctx->x86.pv.shinfo)); These are some very contorted hoops to extend the data size sent. write_split_record() is the the tool to use here, although the above suggestions may render this change unnecessary. > @@ -1041,7 +1036,7 @@ static int x86_pv_setup(struct xc_sr_context *ctx) > if ( rc ) > return rc; > > - rc = map_shinfo(ctx); > + rc = x86_pv_get_shinfo(ctx); > if ( rc ) > return rc; > > @@ -1112,12 +1107,11 @@ static int x86_pv_cleanup(struct xc_sr_context *ctx) > if ( ctx->x86.pv.p2m ) > munmap(ctx->x86.pv.p2m, ctx->x86.pv.p2m_frames * PAGE_SIZE); > > - if ( ctx->x86.pv.shinfo ) > - munmap(ctx->x86.pv.shinfo, PAGE_SIZE); > - > if ( ctx->x86.pv.m2p ) > munmap(ctx->x86.pv.m2p, ctx->x86.pv.nr_m2p_frames * PAGE_SIZE); > > + common_cleanup(ctx); > + > return 0; > } This pair highlights an asymmetry in the patch, which will need fixing by whomever adds a second field to domain_context. Obtaining the context for use should shouldn't be a hidden side effect of getting the shared info. ~Andrew
> -----Original Message----- > From: Andrew Cooper <amc96@hermes.cam.ac.uk> On Behalf Of Andrew Cooper > Sent: 30 May 2020 01:30 > To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org > Cc: Paul Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v6 5/5] tools/libxc: make use of domain context SHARED_INFO record... > > On 27/05/2020 18:34, Paul Durrant wrote: > > ... in the save/restore code. > > > > This patch replaces direct mapping of the shared_info_frame (retrieved > > using XEN_DOMCTL_getdomaininfo) with save/load of the domain context > > SHARED_INFO record. > > > > No modifications are made to the definition of the migration stream at > > this point. Subsequent patches will define a record in the libxc domain > > image format for passing domain context and convert the save/restore code > > to use that. > > > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > I was going to fix up the remaining issues and commit this, but there is > a rather major problem in the way. Therefore, here is a rather more > full review. > Thanks, but I have to say that this is quite late, coming in v6. > > diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c > > index dd9a11b4b5..1acb3765aa 100644 > > --- a/tools/libxc/xc_sr_common.c > > +++ b/tools/libxc/xc_sr_common.c > > @@ -138,6 +138,73 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec) > > return 0; > > }; > > > > +int get_domain_context(struct xc_sr_context *ctx) > > +{ > > + xc_interface *xch = ctx->xch; > > + size_t len = 0; > > + int rc; > > + > > + if ( ctx->domain_context.buffer ) > > + { > > + ERROR("Domain context already present"); > > + return -1; > > + } > > + > > + rc = xc_domain_getcontext(xch, ctx->domid, NULL, &len); > > + if ( rc < 0 ) > > + { > > + PERROR("Unable to get size of domain context"); > > + return -1; > > + } > > + > > + ctx->domain_context.buffer = malloc(len); > > + if ( !ctx->domain_context.buffer ) > > + { > > + PERROR("Unable to allocate memory for domain context"); > > + return -1; > > + } > > There is an xc_sr_blob interface, as this is a common pattern. > Ok. > > + > > + rc = xc_domain_getcontext(xch, ctx->domid, ctx->domain_context.buffer, > > + &len); > > + if ( rc < 0 ) > > + { > > + PERROR("Unable to get domain context"); > > + return -1; > > + } > > + > > + ctx->domain_context.len = len; > > + > > + return 0; > > +} > > + > > +int set_domain_context(struct xc_sr_context *ctx) > > +{ > > + xc_interface *xch = ctx->xch; > > + int rc; > > + > > + if ( !ctx->domain_context.buffer ) > > + { > > + ERROR("Domain context not present"); > > + return -1; > > + } > > + > > + rc = xc_domain_setcontext(xch, ctx->domid, ctx->domain_context.buffer, > > + ctx->domain_context.len); > > + > > + if ( rc < 0 ) > > + { > > + PERROR("Unable to set domain context"); > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +void common_cleanup(struct xc_sr_context *ctx) > > +{ > > + free(ctx->domain_context.buffer); > > There is only a single caller to this function, so there is a (possibly > latent) memory leak. > > > +} > > + > > static void __attribute__((unused)) build_assertions(void) > > { > > BUILD_BUG_ON(sizeof(struct xc_sr_ihdr) != 24); > > diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h > > index 5dd51ccb15..0d61978b08 100644 > > --- a/tools/libxc/xc_sr_common.h > > +++ b/tools/libxc/xc_sr_common.h > > @@ -208,6 +208,11 @@ struct xc_sr_context > > > > xc_dominfo_t dominfo; > > > > + struct { > > + void *buffer; > > + unsigned int len; > > + } domain_context; > > As noted above, xc_sr_blob domain_context; > > > diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c > > index 904ccc462a..21982a38ad 100644 > > --- a/tools/libxc/xc_sr_restore_x86_pv.c > > +++ b/tools/libxc/xc_sr_restore_x86_pv.c > > @@ -865,7 +865,7 @@ static int handle_shared_info(struct xc_sr_context *ctx, > > xc_interface *xch = ctx->xch; > > unsigned int i; > > int rc = -1; > > - shared_info_any_t *guest_shinfo = NULL; > > + shared_info_any_t *guest_shinfo; > > const shared_info_any_t *old_shinfo = rec->data; > > > > if ( !ctx->x86.pv.restore.seen_pv_info ) > > @@ -878,18 +878,14 @@ static int handle_shared_info(struct xc_sr_context *ctx, > > { > > ERROR("X86_PV_SHARED_INFO record wrong size: length %u" > > ", expected 4096", rec->length); > > - goto err; > > + return -1; > > } > > > > - guest_shinfo = xc_map_foreign_range( > > - xch, ctx->domid, PAGE_SIZE, PROT_READ | PROT_WRITE, > > - ctx->dominfo.shared_info_frame); > > - if ( !guest_shinfo ) > > - { > > - PERROR("Failed to map Shared Info at mfn %#lx", > > - ctx->dominfo.shared_info_frame); > > - goto err; > > - } > > + rc = x86_pv_get_shinfo(ctx); > > + if ( rc ) > > + return rc; > > If I'm following this logic correctly, we're now in the final throws of > completing migration with data in hand, and we ask the hypervisor to > gather the entire domain context for this (not-yet-run) guest, copy it > (twice, even) down into userspace, so we can scan through it to find the > appropriate tag, copy less than a page's worth of data, then pass the > full buffer back to Xen to unserialise the whole thing over the guest. > That is the case at the moment yes. I do state quite clearly in the commit comment: "No modifications are made to the definition of the migration stream at this point. Subsequent patches will define a record in the libxc domain image format for passing domain context and convert the save/restore code to use that." > The restore path shouldn't be calling DOMCTL_get* at all. It is > conceptually wrong, and a waste of time/effort which would be better > spent with the guest unpaused. > I refer to the quoted text above. > What the restore path should be doing is passing data from the stream, > straight into DOMCTL_set* (and attempting to do this will probably > demonstrate why hvmctxt was an especially poor API to copy). The layout > of existing migration-v2 blocks was designed around blobs which Xen > produces and consumes directly, specifically to minimise the processing > required. > > I think it is a layering violation to have libxc pick apart and reframe > the internals of another layers' blob. > Again, that was a pragmatic choice at this stage. The layering violation is already there; I have not added code to pick apart the shared info... it was there already. I also don't think picking apart the domain context buffer is necessarily a layering violation. > What is not currently clear is whether the domain context wants sending > as a discrete blob itself (and have a new chunk type allocated for the > purpose), or each bit of it is going to want picking apart. This > largely depends on what else is going in the blob at a Xen level. > > Also, I'd like to see the plans for the HVM side of this logic before > deciding on whether the PV side is appropriate. I know we have a > dedicated SHARED_INFO record right now, but it would be fine (AFAICT) to > relax the migration spec to state that one of {SHARED_INFO, > DOMAIN_CONTEXT} must be sent for PV. > If you object to this interim step then I think I will indeed abstract away from SHARED_INFO instead. Very little of it is actually used in PV migration anyway and different parts will be needed for guest transparent live migration. > > @@ -854,13 +835,27 @@ static int write_x86_pv_p2m_frames(struct xc_sr_context *ctx) > > */ > > static int write_shared_info(struct xc_sr_context *ctx) > > { > > + xc_interface *xch = ctx->xch; > > struct xc_sr_record rec = { > > .type = REC_TYPE_SHARED_INFO, > > .length = PAGE_SIZE, > > - .data = ctx->x86.pv.shinfo, > > }; > > + int rc; > > > > - return write_record(ctx, &rec); > > + if ( !(rec.data = calloc(1, PAGE_SIZE)) ) > > + { > > + ERROR("Cannot allocate buffer for SHARED_INFO data"); > > + return -1; > > + } > > + > > + BUILD_BUG_ON(sizeof(*ctx->x86.pv.shinfo) > PAGE_SIZE); > > + memcpy(rec.data, ctx->x86.pv.shinfo, sizeof(*ctx->x86.pv.shinfo)); > > These are some very contorted hoops to extend the data size sent. > > write_split_record() is the the tool to use here, although the above > suggestions may render this change unnecessary. > Indeed. > > @@ -1041,7 +1036,7 @@ static int x86_pv_setup(struct xc_sr_context *ctx) > > if ( rc ) > > return rc; > > > > - rc = map_shinfo(ctx); > > + rc = x86_pv_get_shinfo(ctx); > > if ( rc ) > > return rc; > > > > @@ -1112,12 +1107,11 @@ static int x86_pv_cleanup(struct xc_sr_context *ctx) > > if ( ctx->x86.pv.p2m ) > > munmap(ctx->x86.pv.p2m, ctx->x86.pv.p2m_frames * PAGE_SIZE); > > > > - if ( ctx->x86.pv.shinfo ) > > - munmap(ctx->x86.pv.shinfo, PAGE_SIZE); > > - > > if ( ctx->x86.pv.m2p ) > > munmap(ctx->x86.pv.m2p, ctx->x86.pv.nr_m2p_frames * PAGE_SIZE); > > > > + common_cleanup(ctx); > > + > > return 0; > > } > > This pair highlights an asymmetry in the patch, which will need fixing > by whomever adds a second field to domain_context. Obtaining the > context for use should shouldn't be a hidden side effect of getting the > shared info. > Ok. Paul > ~Andrew
diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c index dd9a11b4b5..1acb3765aa 100644 --- a/tools/libxc/xc_sr_common.c +++ b/tools/libxc/xc_sr_common.c @@ -138,6 +138,73 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec) return 0; }; +int get_domain_context(struct xc_sr_context *ctx) +{ + xc_interface *xch = ctx->xch; + size_t len = 0; + int rc; + + if ( ctx->domain_context.buffer ) + { + ERROR("Domain context already present"); + return -1; + } + + rc = xc_domain_getcontext(xch, ctx->domid, NULL, &len); + if ( rc < 0 ) + { + PERROR("Unable to get size of domain context"); + return -1; + } + + ctx->domain_context.buffer = malloc(len); + if ( !ctx->domain_context.buffer ) + { + PERROR("Unable to allocate memory for domain context"); + return -1; + } + + rc = xc_domain_getcontext(xch, ctx->domid, ctx->domain_context.buffer, + &len); + if ( rc < 0 ) + { + PERROR("Unable to get domain context"); + return -1; + } + + ctx->domain_context.len = len; + + return 0; +} + +int set_domain_context(struct xc_sr_context *ctx) +{ + xc_interface *xch = ctx->xch; + int rc; + + if ( !ctx->domain_context.buffer ) + { + ERROR("Domain context not present"); + return -1; + } + + rc = xc_domain_setcontext(xch, ctx->domid, ctx->domain_context.buffer, + ctx->domain_context.len); + + if ( rc < 0 ) + { + PERROR("Unable to set domain context"); + return -1; + } + + return 0; +} + +void common_cleanup(struct xc_sr_context *ctx) +{ + free(ctx->domain_context.buffer); +} + static void __attribute__((unused)) build_assertions(void) { BUILD_BUG_ON(sizeof(struct xc_sr_ihdr) != 24); diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h index 5dd51ccb15..0d61978b08 100644 --- a/tools/libxc/xc_sr_common.h +++ b/tools/libxc/xc_sr_common.h @@ -208,6 +208,11 @@ struct xc_sr_context xc_dominfo_t dominfo; + struct { + void *buffer; + unsigned int len; + } domain_context; + union /* Common save or restore data. */ { struct /* Save data. */ @@ -314,7 +319,7 @@ struct xc_sr_context /* The guest pfns containing the p2m leaves */ xen_pfn_t *p2m_pfns; - /* Read-only mapping of guests shared info page */ + /* Pointer to shared_info (located in context buffer) */ shared_info_any_t *shinfo; /* p2m generation count for verifying validity of local p2m. */ @@ -425,6 +430,10 @@ 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); +int get_domain_context(struct xc_sr_context *ctx); +int set_domain_context(struct xc_sr_context *ctx); +void common_cleanup(struct xc_sr_context *ctx); + #endif /* * Local variables: diff --git a/tools/libxc/xc_sr_common_x86_pv.c b/tools/libxc/xc_sr_common_x86_pv.c index d3d425cb82..69d9b142b8 100644 --- a/tools/libxc/xc_sr_common_x86_pv.c +++ b/tools/libxc/xc_sr_common_x86_pv.c @@ -182,6 +182,80 @@ int x86_pv_map_m2p(struct xc_sr_context *ctx) return rc; } +int x86_pv_get_shinfo(struct xc_sr_context *ctx) +{ + xc_interface *xch = ctx->xch; + unsigned int off = 0; + int rc; + +#define GET_PTR(_x) \ + do { \ + if ( ctx->domain_context.len - off < sizeof(*(_x)) ) \ + { \ + ERROR("Need another %lu bytes of context, only %u available\n", \ + sizeof(*(_x)), ctx->domain_context.len - off); \ + return -1; \ + } \ + (_x) = ctx->domain_context.buffer + off; \ + } while (false); + + rc = get_domain_context(ctx); + if ( rc ) + return rc; + + for ( ; ; ) + { + struct domain_save_descriptor *desc; + + GET_PTR(desc); + + off += sizeof(*desc); + + switch (desc->typecode) + { + case DOMAIN_SAVE_CODE(SHARED_INFO): + { + DOMAIN_SAVE_TYPE(SHARED_INFO) *s; + + GET_PTR(s); + + ctx->x86.pv.shinfo = (shared_info_any_t *)s->buffer; + break; + } + default: + break; + } + + if ( desc->typecode == DOMAIN_SAVE_CODE(END) ) + break; + + off += desc->length; + } + + if ( !ctx->x86.pv.shinfo ) + { + ERROR("Failed to get SHARED_INFO\n"); + return -1; + } + + return 0; + +#undef GET_PTR +} + +int x86_pv_set_shinfo(struct xc_sr_context *ctx) +{ + xc_interface *xch = ctx->xch; + + if ( !ctx->x86.pv.shinfo ) + { + ERROR("SHARED_INFO buffer not present\n"); + return -1; + } + + return set_domain_context(ctx); +} + /* * Local variables: * mode: C diff --git a/tools/libxc/xc_sr_common_x86_pv.h b/tools/libxc/xc_sr_common_x86_pv.h index 2ed03309af..01442f48fb 100644 --- a/tools/libxc/xc_sr_common_x86_pv.h +++ b/tools/libxc/xc_sr_common_x86_pv.h @@ -97,6 +97,9 @@ int x86_pv_domain_info(struct xc_sr_context *ctx); */ int x86_pv_map_m2p(struct xc_sr_context *ctx); +int x86_pv_get_shinfo(struct xc_sr_context *ctx); +int x86_pv_set_shinfo(struct xc_sr_context *ctx); + #endif /* * Local variables: diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c index 904ccc462a..21982a38ad 100644 --- a/tools/libxc/xc_sr_restore_x86_pv.c +++ b/tools/libxc/xc_sr_restore_x86_pv.c @@ -865,7 +865,7 @@ static int handle_shared_info(struct xc_sr_context *ctx, xc_interface *xch = ctx->xch; unsigned int i; int rc = -1; - shared_info_any_t *guest_shinfo = NULL; + shared_info_any_t *guest_shinfo; const shared_info_any_t *old_shinfo = rec->data; if ( !ctx->x86.pv.restore.seen_pv_info ) @@ -878,18 +878,14 @@ static int handle_shared_info(struct xc_sr_context *ctx, { ERROR("X86_PV_SHARED_INFO record wrong size: length %u" ", expected 4096", rec->length); - goto err; + return -1; } - guest_shinfo = xc_map_foreign_range( - xch, ctx->domid, PAGE_SIZE, PROT_READ | PROT_WRITE, - ctx->dominfo.shared_info_frame); - if ( !guest_shinfo ) - { - PERROR("Failed to map Shared Info at mfn %#lx", - ctx->dominfo.shared_info_frame); - goto err; - } + rc = x86_pv_get_shinfo(ctx); + if ( rc ) + return rc; + + guest_shinfo = ctx->x86.pv.shinfo; MEMCPY_FIELD(guest_shinfo, old_shinfo, vcpu_info, ctx->x86.pv.width); MEMCPY_FIELD(guest_shinfo, old_shinfo, arch, ctx->x86.pv.width); @@ -904,13 +900,7 @@ static int handle_shared_info(struct xc_sr_context *ctx, MEMSET_ARRAY_FIELD(guest_shinfo, evtchn_mask, 0xff, ctx->x86.pv.width); - rc = 0; - - err: - if ( guest_shinfo ) - munmap(guest_shinfo, PAGE_SIZE); - - return rc; + return x86_pv_set_shinfo(ctx); } /* restore_ops function. */ diff --git a/tools/libxc/xc_sr_save_x86_pv.c b/tools/libxc/xc_sr_save_x86_pv.c index f3ccf5bb4b..fdd172b639 100644 --- a/tools/libxc/xc_sr_save_x86_pv.c +++ b/tools/libxc/xc_sr_save_x86_pv.c @@ -9,25 +9,6 @@ static inline bool is_canonical_address(xen_vaddr_t vaddr) return ((int64_t)vaddr >> 47) == ((int64_t)vaddr >> 63); } -/* - * Maps the guests shared info page. - */ -static int map_shinfo(struct xc_sr_context *ctx) -{ - xc_interface *xch = ctx->xch; - - ctx->x86.pv.shinfo = xc_map_foreign_range( - xch, ctx->domid, PAGE_SIZE, PROT_READ, ctx->dominfo.shared_info_frame); - if ( !ctx->x86.pv.shinfo ) - { - PERROR("Failed to map shared info frame at mfn %#lx", - ctx->dominfo.shared_info_frame); - return -1; - } - - return 0; -} - /* * Copy a list of mfns from a guest, accounting for differences between guest * and toolstack width. Can fail if truncation would occur. @@ -854,13 +835,27 @@ static int write_x86_pv_p2m_frames(struct xc_sr_context *ctx) */ static int write_shared_info(struct xc_sr_context *ctx) { + xc_interface *xch = ctx->xch; struct xc_sr_record rec = { .type = REC_TYPE_SHARED_INFO, .length = PAGE_SIZE, - .data = ctx->x86.pv.shinfo, }; + int rc; - return write_record(ctx, &rec); + if ( !(rec.data = calloc(1, PAGE_SIZE)) ) + { + ERROR("Cannot allocate buffer for SHARED_INFO data"); + return -1; + } + + BUILD_BUG_ON(sizeof(*ctx->x86.pv.shinfo) > PAGE_SIZE); + memcpy(rec.data, ctx->x86.pv.shinfo, sizeof(*ctx->x86.pv.shinfo)); + + rc = write_record(ctx, &rec); + + free(rec.data); + + return rc; } /* @@ -1041,7 +1036,7 @@ static int x86_pv_setup(struct xc_sr_context *ctx) if ( rc ) return rc; - rc = map_shinfo(ctx); + rc = x86_pv_get_shinfo(ctx); if ( rc ) return rc; @@ -1112,12 +1107,11 @@ static int x86_pv_cleanup(struct xc_sr_context *ctx) if ( ctx->x86.pv.p2m ) munmap(ctx->x86.pv.p2m, ctx->x86.pv.p2m_frames * PAGE_SIZE); - if ( ctx->x86.pv.shinfo ) - munmap(ctx->x86.pv.shinfo, PAGE_SIZE); - if ( ctx->x86.pv.m2p ) munmap(ctx->x86.pv.m2p, ctx->x86.pv.nr_m2p_frames * PAGE_SIZE); + common_cleanup(ctx); + return 0; } diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h index 303081df0d..296b523963 100644 --- a/tools/libxc/xg_save_restore.h +++ b/tools/libxc/xg_save_restore.h @@ -19,6 +19,7 @@ #include <xen/foreign/x86_32.h> #include <xen/foreign/x86_64.h> +#include <xen/save.h> /* ** We process save/restore/migrate in batches of pages; the below
... in the save/restore code. This patch replaces direct mapping of the shared_info_frame (retrieved using XEN_DOMCTL_getdomaininfo) with save/load of the domain context SHARED_INFO record. No modifications are made to the definition of the migration stream at this point. Subsequent patches will define a record in the libxc domain image format for passing domain context and convert the save/restore code to use that. Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> NOTE: Ian requested ack from Andrew v5: - Added BUILD_BUG_ON() in write_shared_info() to ensure copied data is not bigger than the record buffer v4: - write_shared_info() now needs to allocate the record data since the shared info buffer is smaller than PAGE_SIZE v3: - Moved basic get/set domain context functions to common code v2: - Re-based (now making use of DOMAIN_SAVE_FLAG_IGNORE) --- tools/libxc/xc_sr_common.c | 67 +++++++++++++++++++++++++++ tools/libxc/xc_sr_common.h | 11 ++++- tools/libxc/xc_sr_common_x86_pv.c | 74 ++++++++++++++++++++++++++++++ tools/libxc/xc_sr_common_x86_pv.h | 3 ++ tools/libxc/xc_sr_restore_x86_pv.c | 26 ++++------- tools/libxc/xc_sr_save_x86_pv.c | 44 ++++++++---------- tools/libxc/xg_save_restore.h | 1 + 7 files changed, 182 insertions(+), 44 deletions(-)