[Xen-devel] [PATCH RFC 06/20] libxc/xc_sr: factor helpers out of handle_page_data()

[Joshua Otto <jtotto@xxxxxxxxxxxx>]

When processing a PAGE_DATA record, the restore code:
1) applies a number of sanity checks on the record's headers and size
2) decodes the list of packed page info into pfns and their types
3) using the pfn and type info, populates and fills the pages into the
   guest using process_page_data()

Steps 1) and 2) are also useful at various other stages of postcopy live
migrations, so factor them into reusable helper routines.

No functional change.

Signed-off-by: Joshua Otto <jtotto@xxxxxxxxxxxx>
---
 tools/libxc/xc_sr_common.c        | 38 +++++++++++++++-
 tools/libxc/xc_sr_common.h        | 10 +++++
 tools/libxc/xc_sr_restore.c       | 94 ++++++++++++++++++++++++---------------
 tools/libxc/xc_sr_save.c          |  2 +-
 tools/libxc/xc_sr_stream_format.h |  6 +--
 5 files changed, 109 insertions(+), 41 deletions(-)

diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index c1babf6..f443974 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -140,13 +140,49 @@ int read_record(struct xc_sr_context *ctx, int fd, struct 
xc_sr_record *rec)
     return 0;
 };
 
+int validate_pages_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
+                          uint32_t expected_type)
+{
+    xc_interface *xch = ctx->xch;
+    struct xc_sr_rec_pages_header *pages = rec->data;
+
+    if ( rec->type != expected_type )
+    {
+        ERROR("%s record type expected, instead received record of type "
+              "%08x (%s)", rec_type_to_str(expected_type), rec->type,
+              rec_type_to_str(rec->type));
+        return -1;
+    }
+    else if ( rec->length < sizeof(*pages) )
+    {
+        ERROR("%s record truncated: length %u, min %zu",
+              rec_type_to_str(rec->type), rec->length, sizeof(*pages));
+        return -1;
+    }
+    else if ( pages->count < 1 )
+    {
+        ERROR("Expected at least 1 pfn in %s record",
+              rec_type_to_str(rec->type));
+        return -1;
+    }
+    else if ( rec->length < sizeof(*pages) + (pages->count * sizeof(uint64_t)) 
)
+    {
+        ERROR("%s record (length %u) too short to contain %u"
+              " pfns worth of information", rec_type_to_str(rec->type),
+              rec->length, pages->count);
+        return -1;
+    }
+
+    return 0;
+}
+
 static void __attribute__((unused)) build_assertions(void)
 {
     BUILD_BUG_ON(sizeof(struct xc_sr_ihdr) != 24);
     BUILD_BUG_ON(sizeof(struct xc_sr_dhdr) != 16);
     BUILD_BUG_ON(sizeof(struct xc_sr_rhdr) != 8);
 
-    BUILD_BUG_ON(sizeof(struct xc_sr_rec_page_data_header)  != 8);
+    BUILD_BUG_ON(sizeof(struct xc_sr_rec_pages_header)      != 8);
     BUILD_BUG_ON(sizeof(struct xc_sr_rec_x86_pv_info)       != 8);
     BUILD_BUG_ON(sizeof(struct xc_sr_rec_x86_pv_p2m_frames) != 8);
     BUILD_BUG_ON(sizeof(struct xc_sr_rec_x86_pv_vcpu_hdr)   != 8);
diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 2f33ccc..b1aa88e 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -392,6 +392,16 @@ static inline int write_record(struct xc_sr_context *ctx, 
int fd,
 int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec);
 
 /*
+ * Given a record of one of the page data types, validate it by:
+ * - checking its actual type against its specific expected type
+ * - sanity checking its actual length against its claimed length
+ *
+ * Returns 0 on success and non-0 on failure.
+ */
+int validate_pages_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
+                          uint32_t expected_type);
+
+/*
  * This would ideally be private in restore.c, but is needed by
  * x86_pv_localise_page() if we receive pagetables frames ahead of the
  * contents of the frames they point at.
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 8574ee8..4e3c472 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -376,39 +376,25 @@ static int process_page_data(struct xc_sr_context *ctx, 
unsigned count,
 }
 
 /*
- * Validate a PAGE_DATA record from the stream, and pass the results to
- * process_page_data() to actually perform the legwork.
+ * Given a PAGE_DATA record, decode each packed entry into its encoded pfn and
+ * type, storing the results in newly-allocated pfns and types buffers that the
+ * caller must later free().  *pfns and *types may safely be free()ed even 
after
+ * failure.
  */
-static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record 
*rec)
+static int decode_pages_record(struct xc_sr_context *ctx,
+                               struct xc_sr_rec_pages_header *pages,
+                               /* OUT */ xen_pfn_t **pfns,
+                               /* OUT */ uint32_t **types,
+                               /* OUT */ unsigned *pages_of_data)
 {
     xc_interface *xch = ctx->xch;
-    struct xc_sr_rec_page_data_header *pages = rec->data;
-    unsigned i, pages_of_data = 0;
-    int rc = -1;
-
-    xen_pfn_t *pfns = NULL, pfn;
-    uint32_t *types = NULL, type;
-
-    if ( rec->length < sizeof(*pages) )
-    {
-        ERROR("PAGE_DATA record truncated: length %u, min %zu",
-              rec->length, sizeof(*pages));
-        goto err;
-    }
-    else if ( pages->count < 1 )
-    {
-        ERROR("Expected at least 1 pfn in PAGE_DATA record");
-        goto err;
-    }
-    else if ( rec->length < sizeof(*pages) + (pages->count * sizeof(uint64_t)) 
)
-    {
-        ERROR("PAGE_DATA record (length %u) too short to contain %u"
-              " pfns worth of information", rec->length, pages->count);
-        goto err;
-    }
+    unsigned i;
+    xen_pfn_t pfn;
+    uint32_t type;
 
-    pfns = malloc(pages->count * sizeof(*pfns));
-    types = malloc(pages->count * sizeof(*types));
+    *pfns = malloc(pages->count * sizeof(*pfns));
+    *types = malloc(pages->count * sizeof(*types));
+    *pages_of_data = 0;
     if ( !pfns || !types )
     {
         ERROR("Unable to allocate enough memory for %u pfns",
@@ -418,14 +404,14 @@ static int handle_page_data(struct xc_sr_context *ctx, 
struct xc_sr_record *rec)
 
     for ( i = 0; i < pages->count; ++i )
     {
-        pfn = pages->pfn[i] & PAGE_DATA_PFN_MASK;
+        pfn = pages->pfn[i] & REC_PFINFO_PFN_MASK;
         if ( !ctx->restore.ops.pfn_is_valid(ctx, pfn) )
         {
             ERROR("pfn %#"PRIpfn" (index %u) outside domain maximum", pfn, i);
             goto err;
         }
 
-        type = (pages->pfn[i] & PAGE_DATA_TYPE_MASK) >> 32;
+        type = (pages->pfn[i] & REC_PFINFO_TYPE_MASK) >> 32;
         if ( ((type >> XEN_DOMCTL_PFINFO_LTAB_SHIFT) >= 5) &&
              ((type >> XEN_DOMCTL_PFINFO_LTAB_SHIFT) <= 8) )
         {
@@ -434,14 +420,50 @@ static int handle_page_data(struct xc_sr_context *ctx, 
struct xc_sr_record *rec)
             goto err;
         }
         else if ( type < XEN_DOMCTL_PFINFO_BROKEN )
-            /* NOTAB and all L1 through L4 tables (including pinned) should
-             * have a page worth of data in the record. */
-            pages_of_data++;
+            /* NOTAB and all L1 through L4 tables (including pinned) require 
the
+             * migration of a page of real data. */
+            (*pages_of_data)++;
 
-        pfns[i] = pfn;
-        types[i] = type;
+        (*pfns)[i] = pfn;
+        (*types)[i] = type;
     }
 
+    return 0;
+
+ err:
+    free(*pfns);
+    *pfns = NULL;
+
+    free(*types);
+    *types = NULL;
+
+    *pages_of_data = 0;
+
+    return -1;
+}
+
+/*
+ * Validate a PAGE_DATA record from the stream, and pass the results to
+ * process_page_data() to actually perform the legwork.
+ */
+static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record 
*rec)
+{
+    xc_interface *xch = ctx->xch;
+    struct xc_sr_rec_pages_header *pages = rec->data;
+    unsigned pages_of_data;
+    int rc = -1;
+
+    xen_pfn_t *pfns = NULL;
+    uint32_t *types = NULL;
+
+    rc = validate_pages_record(ctx, rec, REC_TYPE_PAGE_DATA);
+    if ( rc )
+        goto err;
+
+    rc = decode_pages_record(ctx, pages, &pfns, &types, &pages_of_data);
+    if ( rc )
+        goto err;
+
     if ( rec->length != (sizeof(*pages) +
                          (sizeof(uint64_t) * pages->count) +
                          (PAGE_SIZE * pages_of_data)) )
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 86f6903..797aec5 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -83,7 +83,7 @@ static int write_batch(struct xc_sr_context *ctx)
     void *page, *orig_page;
     uint64_t *rec_pfns = NULL;
     struct iovec *iov = NULL; int iovcnt = 0;
-    struct xc_sr_rec_page_data_header hdr = { 0 };
+    struct xc_sr_rec_pages_header hdr = { 0 };
     struct xc_sr_record rec =
     {
         .type = REC_TYPE_PAGE_DATA,
diff --git a/tools/libxc/xc_sr_stream_format.h 
b/tools/libxc/xc_sr_stream_format.h
index 3291b25..32400b2 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -80,15 +80,15 @@ struct xc_sr_rhdr
 #define REC_TYPE_OPTIONAL             0x80000000U
 
 /* PAGE_DATA */
-struct xc_sr_rec_page_data_header
+struct xc_sr_rec_pages_header
 {
     uint32_t count;
     uint32_t _res1;
     uint64_t pfn[0];
 };
 
-#define PAGE_DATA_PFN_MASK  0x000fffffffffffffULL
-#define PAGE_DATA_TYPE_MASK 0xf000000000000000ULL
+#define REC_PFINFO_PFN_MASK  0x000fffffffffffffULL
+#define REC_PFINFO_TYPE_MASK 0xf000000000000000ULL
 
 /* X86_PV_INFO */
 struct xc_sr_rec_x86_pv_info
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel