diff mbox

[RFC,06/20] libxc/xc_sr: factor helpers out of handle_page_data()

Message ID 1490605592-12189-7-git-send-email-jtotto@uwaterloo.ca (mailing list archive)
State New, archived
Headers show

Commit Message

Joshua Otto March 27, 2017, 9:06 a.m. UTC
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@uwaterloo.ca>
---
 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(-)

Comments

Andrew Cooper March 28, 2017, 7:52 p.m. UTC | #1
On 27/03/17 10:06, Joshua Otto wrote:
> 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

What are the purposes of these name changes?

~Andrew
Joshua Otto March 30, 2017, 4:49 a.m. UTC | #2
On Tue, Mar 28, 2017 at 08:52:26PM +0100, Andrew Cooper wrote:
> On 27/03/17 10:06, Joshua Otto wrote:
> > 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
> 
> What are the purposes of these name changes?

I should definitely have explained this more explicitly, sorry about that.  I
use the same exact structure (a count followed by a list of encoded pfns+types)
for three additional record types (POSTCOPY_PFNS, POSTCOPY_PAGE_DATA, and
POSTCOPY_FAULT) later in the series when postcopy is introduced.  To enable the
generation and validation logic to be shared between all of the code that
processes this sort of record, I renamed the structure and its associated masks
to be more generic.

Josh
Wei Liu April 12, 2017, 3:16 p.m. UTC | #3
On Thu, Mar 30, 2017 at 12:49:07AM -0400, Joshua Otto wrote:
> On Tue, Mar 28, 2017 at 08:52:26PM +0100, Andrew Cooper wrote:
> > On 27/03/17 10:06, Joshua Otto wrote:
> > > 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
> > 
> > What are the purposes of these name changes?
> 
> I should definitely have explained this more explicitly, sorry about that.  I
> use the same exact structure (a count followed by a list of encoded pfns+types)
> for three additional record types (POSTCOPY_PFNS, POSTCOPY_PAGE_DATA, and
> POSTCOPY_FAULT) later in the series when postcopy is introduced.  To enable the
> generation and validation logic to be shared between all of the code that
> processes this sort of record, I renamed the structure and its associated masks
> to be more generic.

This should be part of the commit message.

Wei.

> 
> Josh
diff mbox

Patch

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