diff mbox series

[02/12] libxc/restore: Introduce functionality to simplify blob handling

Message ID 20191224151932.6304-3-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series Support CPUID/MSR data in migration streams | expand

Commit Message

Andrew Cooper Dec. 24, 2019, 3:19 p.m. UTC
During migration, we buffer several blobs of data which ultimately need
handing back to Xen at an appropriate time.

Currently, this is all handled in an ad-hoc manner, but more blobs are soon
going to be added.  Introduce xc_sr_blob to encapsulate a ptr/size pair, and
update_blob() to handle the memory management aspects.

Switch the HVM_CONTEXT and the four PV_VCPU_* blobs over to this new
infrastructure.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 tools/libxc/xc_sr_common.h          | 45 ++++++++++++++++++++-----
 tools/libxc/xc_sr_restore_x86_hvm.c | 21 ++++--------
 tools/libxc/xc_sr_restore_x86_pv.c  | 67 ++++++++++++++-----------------------
 3 files changed, 68 insertions(+), 65 deletions(-)

Comments

Ian Jackson Jan. 14, 2020, 4:50 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 02/12] libxc/restore: Introduce functionality to simplify blob handling"):
> During migration, we buffer several blobs of data which ultimately need
> handing back to Xen at an appropriate time.
> 
> Currently, this is all handled in an ad-hoc manner, but more blobs are soon
> going to be added.  Introduce xc_sr_blob to encapsulate a ptr/size pair, and
> update_blob() to handle the memory management aspects.
> 
> Switch the HVM_CONTEXT and the four PV_VCPU_* blobs over to this new
> infrastructure.

I like the new approach.

I was expecting to see "no functional change in the commit message".

If you think that is true, and with that added,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.
diff mbox series

Patch

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index b66d785e50..19b053911f 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -165,13 +165,40 @@  struct xc_sr_restore_ops
     int (*cleanup)(struct xc_sr_context *ctx);
 };
 
-/* x86 PV per-vcpu storage structure for blobs heading Xen-wards. */
-struct xc_sr_x86_pv_restore_vcpu
+/* Wrapper for blobs of data heading Xen-wards. */
+struct xc_sr_blob
 {
-    void *basic, *extd, *xsave, *msr;
-    size_t basicsz, extdsz, xsavesz, msrsz;
+    void *ptr;
+    size_t size;
 };
 
+/*
+ * Update a blob.  Duplicate src/size, freeing the old blob if necessary.  May
+ * fail due to memory allocation.
+ */
+static inline int update_blob(struct xc_sr_blob *blob,
+                              const void *src, size_t size)
+{
+    void *ptr;
+
+    if ( !src || !size )
+    {
+        errno = EINVAL;
+        return -1;
+    }
+
+    if ( (ptr = malloc(size)) == NULL )
+        return -1;
+
+    memcpy(ptr, src, size);
+
+    free(blob->ptr);
+    blob->ptr = memcpy(ptr, src, size);
+    blob->size = size;
+
+    return 0;
+}
+
 struct xc_sr_context
 {
     xc_interface *xch;
@@ -306,8 +333,11 @@  struct xc_sr_context
                     /* Types for each page (bounded by max_pfn). */
                     uint32_t *pfn_types;
 
-                    /* Vcpu context blobs. */
-                    struct xc_sr_x86_pv_restore_vcpu *vcpus;
+                    /* x86 PV per-vcpu storage structure for blobs. */
+                    struct xc_sr_x86_pv_restore_vcpu
+                    {
+                        struct xc_sr_blob basic, extd, xsave, msr;
+                    } *vcpus;
                     unsigned int nr_vcpus;
                 } restore;
             };
@@ -327,8 +357,7 @@  struct xc_sr_context
                 struct
                 {
                     /* HVM context blob. */
-                    void *context;
-                    size_t contextsz;
+                    struct xc_sr_blob context;
                 } restore;
             };
         } x86_hvm;
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 4a24dc0137..fe7be9bde6 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -10,21 +10,12 @@  static int handle_hvm_context(struct xc_sr_context *ctx,
                               struct xc_sr_record *rec)
 {
     xc_interface *xch = ctx->xch;
-    void *p;
+    int rc = update_blob(&ctx->x86_hvm.restore.context, rec->data, rec->length);
 
-    p = malloc(rec->length);
-    if ( !p )
-    {
+    if ( rc )
         ERROR("Unable to allocate %u bytes for hvm context", rec->length);
-        return -1;
-    }
 
-    free(ctx->x86_hvm.restore.context);
-
-    ctx->x86_hvm.restore.context = memcpy(p, rec->data, rec->length);
-    ctx->x86_hvm.restore.contextsz = rec->length;
-
-    return 0;
+    return rc;
 }
 
 /*
@@ -210,8 +201,8 @@  static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
     }
 
     rc = xc_domain_hvm_setcontext(xch, ctx->domid,
-                                  ctx->x86_hvm.restore.context,
-                                  ctx->x86_hvm.restore.contextsz);
+                                  ctx->x86_hvm.restore.context.ptr,
+                                  ctx->x86_hvm.restore.context.size);
     if ( rc < 0 )
     {
         PERROR("Unable to restore HVM context");
@@ -234,7 +225,7 @@  static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
 
 static int x86_hvm_cleanup(struct xc_sr_context *ctx)
 {
-    free(ctx->x86_hvm.restore.context);
+    free(ctx->x86_hvm.restore.context.ptr);
 
     return 0;
 }
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index c0598af8b7..0ec506632a 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -236,7 +236,7 @@  static int process_vcpu_basic(struct xc_sr_context *ctx,
                               unsigned int vcpuid)
 {
     xc_interface *xch = ctx->xch;
-    vcpu_guest_context_any_t *vcpu = ctx->x86_pv.restore.vcpus[vcpuid].basic;
+    vcpu_guest_context_any_t *vcpu = ctx->x86_pv.restore.vcpus[vcpuid].basic.ptr;
     xen_pfn_t pfn, mfn;
     unsigned int i, gdt_count;
     int rc = -1;
@@ -380,7 +380,7 @@  static int process_vcpu_extended(struct xc_sr_context *ctx,
 
     domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
     domctl.domain = ctx->domid;
-    memcpy(&domctl.u.ext_vcpucontext, vcpu->extd, vcpu->extdsz);
+    memcpy(&domctl.u.ext_vcpucontext, vcpu->extd.ptr, vcpu->extd.size);
 
     if ( xc_domctl(xch, &domctl) != 0 )
     {
@@ -404,21 +404,21 @@  static int process_vcpu_xsave(struct xc_sr_context *ctx,
     DECLARE_DOMCTL;
     DECLARE_HYPERCALL_BUFFER(void, buffer);
 
-    buffer = xc_hypercall_buffer_alloc(xch, buffer, vcpu->xsavesz);
+    buffer = xc_hypercall_buffer_alloc(xch, buffer, vcpu->xsave.size);
     if ( !buffer )
     {
         ERROR("Unable to allocate %zu bytes for xsave hypercall buffer",
-              vcpu->xsavesz);
+              vcpu->xsave.size);
         return -1;
     }
 
     domctl.cmd = XEN_DOMCTL_setvcpuextstate;
     domctl.domain = ctx->domid;
     domctl.u.vcpuextstate.vcpu = vcpuid;
-    domctl.u.vcpuextstate.size = vcpu->xsavesz;
+    domctl.u.vcpuextstate.size = vcpu->xsave.size;
     set_xen_guest_handle(domctl.u.vcpuextstate.buffer, buffer);
 
-    memcpy(buffer, vcpu->xsave, vcpu->xsavesz);
+    memcpy(buffer, vcpu->xsave.ptr, vcpu->xsave.size);
 
     rc = xc_domctl(xch, &domctl);
     if ( rc )
@@ -442,21 +442,21 @@  static int process_vcpu_msrs(struct xc_sr_context *ctx,
     DECLARE_DOMCTL;
     DECLARE_HYPERCALL_BUFFER(void, buffer);
 
-    buffer = xc_hypercall_buffer_alloc(xch, buffer, vcpu->msrsz);
+    buffer = xc_hypercall_buffer_alloc(xch, buffer, vcpu->msr.size);
     if ( !buffer )
     {
         ERROR("Unable to allocate %zu bytes for msr hypercall buffer",
-              vcpu->msrsz);
+              vcpu->msr.size);
         return -1;
     }
 
     domctl.cmd = XEN_DOMCTL_set_vcpu_msrs;
     domctl.domain = ctx->domid;
     domctl.u.vcpu_msrs.vcpu = vcpuid;
-    domctl.u.vcpu_msrs.msr_count = vcpu->msrsz / sizeof(xen_domctl_vcpu_msr_t);
+    domctl.u.vcpu_msrs.msr_count = vcpu->msr.size / sizeof(xen_domctl_vcpu_msr_t);
     set_xen_guest_handle(domctl.u.vcpu_msrs.msrs, buffer);
 
-    memcpy(buffer, vcpu->msr, vcpu->msrsz);
+    memcpy(buffer, vcpu->msr.ptr, vcpu->msr.size);
 
     rc = xc_domctl(xch, &domctl);
     if ( rc )
@@ -481,7 +481,7 @@  static int update_vcpu_context(struct xc_sr_context *ctx)
     {
         vcpu = &ctx->x86_pv.restore.vcpus[i];
 
-        if ( vcpu->basic )
+        if ( vcpu->basic.ptr )
         {
             rc = process_vcpu_basic(ctx, i);
             if ( rc )
@@ -493,21 +493,21 @@  static int update_vcpu_context(struct xc_sr_context *ctx)
             return -1;
         }
 
-        if ( vcpu->extd )
+        if ( vcpu->extd.ptr )
         {
             rc = process_vcpu_extended(ctx, i);
             if ( rc )
                 return rc;
         }
 
-        if ( vcpu->xsave )
+        if ( vcpu->xsave.ptr )
         {
             rc = process_vcpu_xsave(ctx, i);
             if ( rc )
                 return rc;
         }
 
-        if ( vcpu->msr )
+        if ( vcpu->msr.ptr )
         {
             rc = process_vcpu_msrs(ctx, i);
             if ( rc )
@@ -738,7 +738,7 @@  static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
     struct xc_sr_x86_pv_restore_vcpu *vcpu;
     const char *rec_name;
     size_t blobsz;
-    void *blob;
+    struct xc_sr_blob *blob;
     int rc = -1;
 
     switch ( rec->type )
@@ -812,6 +812,7 @@  static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
                   rec_name, sizeof(*vhdr) + vcpusz, rec->length);
             goto out;
         }
+        blob = &vcpu->basic;
         break;
     }
 
@@ -822,6 +823,7 @@  static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
                   rec_name, sizeof(*vhdr) + 128, rec->length);
             goto out;
         }
+        blob = &vcpu->extd;
         break;
 
     case REC_TYPE_X86_PV_VCPU_XSAVE:
@@ -831,6 +833,7 @@  static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
                   rec_name, sizeof(*vhdr) + 128, rec->length);
             goto out;
         }
+        blob = &vcpu->xsave;
         break;
 
     case REC_TYPE_X86_PV_VCPU_MSRS:
@@ -840,34 +843,14 @@  static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
                   rec_name, blobsz, sizeof(xen_domctl_vcpu_msr_t));
             goto out;
         }
+        blob = &vcpu->msr;
         break;
     }
 
-    /* Allocate memory. */
-    blob = malloc(blobsz);
-    if ( !blob )
-    {
+    rc = update_blob(blob, vhdr->context, blobsz);
+    if ( rc )
         ERROR("Unable to allocate %zu bytes for vcpu%u %s blob",
               blobsz, vhdr->vcpu_id, rec_name);
-        goto out;
-    }
-
-    memcpy(blob, &vhdr->context, blobsz);
-
-    /* Stash sideways for later. */
-    switch ( rec->type )
-    {
-#define RECSTORE(x, y) case REC_TYPE_X86_PV_ ## x: \
-        free(y); (y) = blob; (y ## sz) = blobsz; break
-
-        RECSTORE(VCPU_BASIC,    vcpu->basic);
-        RECSTORE(VCPU_EXTENDED, vcpu->extd);
-        RECSTORE(VCPU_XSAVE,    vcpu->xsave);
-        RECSTORE(VCPU_MSRS,     vcpu->msr);
-#undef RECSTORE
-    }
-
-    rc = 0;
 
  out:
     return rc;
@@ -1159,10 +1142,10 @@  static int x86_pv_cleanup(struct xc_sr_context *ctx)
             struct xc_sr_x86_pv_restore_vcpu *vcpu =
                 &ctx->x86_pv.restore.vcpus[i];
 
-            free(vcpu->basic);
-            free(vcpu->extd);
-            free(vcpu->xsave);
-            free(vcpu->msr);
+            free(vcpu->basic.ptr);
+            free(vcpu->extd.ptr);
+            free(vcpu->xsave.ptr);
+            free(vcpu->msr.ptr);
         }
 
         free(ctx->x86_pv.restore.vcpus);