[PATCH 0/5] tools/migration: Py3 fixes, and page type helpers

[PATCH 0/5] tools/migration: Py3 fixes, and page type helpers

[Andrew Cooper]

FAOD, this is what I'm planning to push, unless anyone shouts urgently.

Olaf Hering (5):
  tools/python: handle libxl__physmap_info.name properly in convert-legacy-stream
  tools/python: fix Python3.4 TypeError in format string
  tools/migration: unify known page type checking
  tools/migration: unify type checking for data pfns in the VM
  tools/migration: unify type checking for data pfns in migration stream

 tools/libs/guest/xg_sr_common.h            | 85 ++++++++++++++++++++++++++++++
 tools/libs/guest/xg_sr_restore.c           | 38 +++----------
 tools/libs/guest/xg_sr_save.c              | 18 +++----
 tools/python/scripts/convert-legacy-stream |  8 +--
 4 files changed, 102 insertions(+), 47 deletions(-)

-- 
2.11.0

[PATCH 1/5] tools/python: handle libxl__physmap_info.name properly in convert-legacy-stream

[Andrew Cooper]

From: Olaf Hering <olaf@aepfle.de>

The trailing member name[] in libxl__physmap_info is written as a
cstring into the stream. The current code does a sanity check if the
last byte is zero. This attempt fails with python3 because name[-1]
returns a type int. As a result the comparison with byte(\00) fails:

  File "/usr/lib/xen/bin/convert-legacy-stream", line 347, in read_libxl_toolstack
    raise StreamError("physmap name not NUL terminated")
  StreamError: physmap name not NUL terminated

To handle both python variants, cast to bytearray().

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/python/scripts/convert-legacy-stream | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
index 66ee3d2f5d29..227e1b5c3fd3 100755
--- a/tools/python/scripts/convert-legacy-stream
+++ b/tools/python/scripts/convert-legacy-stream
@@ -343,7 +343,7 @@ def read_libxl_toolstack(vm, data):
         if twidth == 64:
             name = name[:-4]
 
-        if name[-1] != b'\x00':
+        if bytearray(name)[-1] != 0:
             raise StreamError("physmap name not NUL terminated")
 
         root = b"physmap/%x" % (phys, )
-- 
2.11.0

[PATCH 2/5] tools/python: fix Python3.4 TypeError in format string

[Andrew Cooper]

From: Olaf Hering <olaf@aepfle.de>

Using the first element of a tuple for a format specifier fails with
python3.4 as included in SLE12:
    b = b"string/%x" % (i, )
TypeError: unsupported operand type(s) for %: 'bytes' and 'tuple'

It happens to work with python 2.7 and 3.6.
To support older Py3, format as strings and explicitly encode as ASCII.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/python/scripts/convert-legacy-stream | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
index 227e1b5c3fd3..7fe375a66810 100755
--- a/tools/python/scripts/convert-legacy-stream
+++ b/tools/python/scripts/convert-legacy-stream
@@ -346,9 +346,9 @@ def read_libxl_toolstack(vm, data):
         if bytearray(name)[-1] != 0:
             raise StreamError("physmap name not NUL terminated")
 
-        root = b"physmap/%x" % (phys, )
-        kv = [root + b"/start_addr", b"%x" % (start, ),
-              root + b"/size",       b"%x" % (size, ),
+        root = ("physmap/%x" % (phys, )).encode('ascii')
+        kv = [root + b"/start_addr", ("%x" % (start, )).encode('ascii'),
+              root + b"/size",       ("%x" % (size, )).encode('ascii'),
               root + b"/name",       name[:-1]]
 
         for key, val in zip(kv[0::2], kv[1::2]):
-- 
2.11.0

[PATCH 3/5] tools/migration: unify known page type checking

[Andrew Cooper]

From: Olaf Hering <olaf@aepfle.de>

Users of xc_get_pfn_type_batch may want to sanity check the data
returned by Xen. Add helpers for this purpose:

is_known_page_type verifies the type returned by Xen on the saving
side, or the incoming type for a page on the restoring side, is known
by the save/restore code.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libs/guest/xg_sr_common.h  | 29 +++++++++++++++++++++++++++++
 tools/libs/guest/xg_sr_restore.c |  5 ++---
 tools/libs/guest/xg_sr_save.c    |  6 ++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index e2994e18acf1..e8436d1abb14 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -455,6 +455,35 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count,
 /* Handle a STATIC_DATA_END record. */
 int handle_static_data_end(struct xc_sr_context *ctx);
 
+/* Page type known to the migration logic? */
+static inline bool is_known_page_type(uint32_t type)
+{
+    switch ( type )
+    {
+    case XEN_DOMCTL_PFINFO_NOTAB:
+
+    case XEN_DOMCTL_PFINFO_L1TAB:
+    case XEN_DOMCTL_PFINFO_L1TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L2TAB:
+    case XEN_DOMCTL_PFINFO_L2TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L3TAB:
+    case XEN_DOMCTL_PFINFO_L3TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L4TAB:
+    case XEN_DOMCTL_PFINFO_L4TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_XTAB:
+    case XEN_DOMCTL_PFINFO_XALLOC: /* Synthetic type in Xen 4.2 - 4.5 */
+    case XEN_DOMCTL_PFINFO_BROKEN:
+        return true;
+
+    default:
+        return false;
+    }
+}
+
 #endif
 /*
  * Local variables:
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index b57a7875192f..508953fd3cf7 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -406,10 +406,9 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
         }
 
         type = (pages->pfn[i] & PAGE_DATA_TYPE_MASK) >> 32;
-        if ( ((type >> XEN_DOMCTL_PFINFO_LTAB_SHIFT) >= 5) &&
-             ((type >> XEN_DOMCTL_PFINFO_LTAB_SHIFT) <= 8) )
+        if ( !is_known_page_type(type) )
         {
-            ERROR("Invalid type %#"PRIx32" for pfn %#"PRIpfn" (index %u)",
+            ERROR("Unknown type %#"PRIx32" for pfn %#"PRIpfn" (index %u)",
                   type, pfn, i);
             goto err;
         }
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index f0e2bd048d37..c84a073e86f4 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -147,6 +147,12 @@ static int write_batch(struct xc_sr_context *ctx)
 
     for ( i = 0; i < nr_pfns; ++i )
     {
+        if ( !is_known_page_type(types[i]) )
+        {
+            ERROR("Unknown type %#"PRIpfn" for pfn %#"PRIpfn, types[i], mfns[i]);
+            goto err;
+        }
+
         switch ( types[i] )
         {
         case XEN_DOMCTL_PFINFO_BROKEN:
-- 
2.11.0

[PATCH 4/5] tools/migration: unify type checking for data pfns in the VM

[Andrew Cooper]

From: Olaf Hering <olaf@aepfle.de>

Introduce a helper which decides if a given pfn in the migration
stream is backed by memory.

This highlights more clearly that type XEN_DOMCTL_PFINFO_XALLOC (a
synthetic toolstack-only type used between Xen 4.2 to 4.5 which
indicated a dirty page on the sending side for which no data will be
send in the initial iteration) does get populated in the VM.

No change in behaviour intended, except for invalid page types which now
have a safer default.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libs/guest/xg_sr_common.h  | 29 +++++++++++++++++++++++++++++
 tools/libs/guest/xg_sr_restore.c |  4 +---
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index e8436d1abb14..7f4b0439f6bd 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -484,6 +484,35 @@ static inline bool is_known_page_type(uint32_t type)
     }
 }
 
+/* Page type backed by RAM in the guest? */
+static inline bool page_type_to_populate(uint32_t type)
+{
+    switch ( type )
+    {
+    case XEN_DOMCTL_PFINFO_NOTAB:
+
+    case XEN_DOMCTL_PFINFO_L1TAB:
+    case XEN_DOMCTL_PFINFO_L1TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L2TAB:
+    case XEN_DOMCTL_PFINFO_L2TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L3TAB:
+    case XEN_DOMCTL_PFINFO_L3TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L4TAB:
+    case XEN_DOMCTL_PFINFO_L4TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_XALLOC:
+        return true;
+
+    case XEN_DOMCTL_PFINFO_XTAB:
+    case XEN_DOMCTL_PFINFO_BROKEN:
+    default:
+        return false;
+    }
+}
+
 #endif
 /*
  * Local variables:
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index 508953fd3cf7..3d63442d4537 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -152,9 +152,7 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count,
 
     for ( i = 0; i < count; ++i )
     {
-        if ( (!types || (types &&
-                         (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
-                          types[i] != XEN_DOMCTL_PFINFO_BROKEN))) &&
+        if ( (!types || page_type_to_populate(types[i])) &&
              !pfn_is_populated(ctx, original_pfns[i]) )
         {
             rc = pfn_set_populated(ctx, original_pfns[i]);
-- 
2.11.0

[PATCH 5/5] tools/migration: unify type checking for data pfns in migration stream

[Andrew Cooper]

From: Olaf Hering <olaf@aepfle.de>

Introduce a helper which decides if a given pfn type has data
in the migration stream.

No change in behaviour intended, except for invalid page types which now
have a safer default.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libs/guest/xg_sr_common.h  | 27 +++++++++++++++++++++++++++
 tools/libs/guest/xg_sr_restore.c | 29 +++--------------------------
 tools/libs/guest/xg_sr_save.c    | 14 ++------------
 3 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index 7f4b0439f6bd..36d45ef56fb8 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -513,6 +513,33 @@ static inline bool page_type_to_populate(uint32_t type)
     }
 }
 
+static inline bool page_type_has_stream_data(uint32_t type)
+{
+    switch ( type )
+    {
+    case XEN_DOMCTL_PFINFO_NOTAB:
+
+    case XEN_DOMCTL_PFINFO_L1TAB:
+    case XEN_DOMCTL_PFINFO_L1TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L2TAB:
+    case XEN_DOMCTL_PFINFO_L2TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L3TAB:
+    case XEN_DOMCTL_PFINFO_L3TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+
+    case XEN_DOMCTL_PFINFO_L4TAB:
+    case XEN_DOMCTL_PFINFO_L4TAB | XEN_DOMCTL_PFINFO_LPINTAB:
+        return true;
+
+    case XEN_DOMCTL_PFINFO_XTAB:
+    case XEN_DOMCTL_PFINFO_BROKEN:
+    case XEN_DOMCTL_PFINFO_XALLOC:
+    default:
+        return false;
+    }
+}
+
 #endif
 /*
  * Local variables:
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index 3d63442d4537..aa4113d7f679 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -231,25 +231,8 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
     {
         ctx->restore.ops.set_page_type(ctx, pfns[i], types[i]);
 
-        switch ( types[i] )
-        {
-        case XEN_DOMCTL_PFINFO_NOTAB:
-
-        case XEN_DOMCTL_PFINFO_L1TAB:
-        case XEN_DOMCTL_PFINFO_L1TAB | XEN_DOMCTL_PFINFO_LPINTAB:
-
-        case XEN_DOMCTL_PFINFO_L2TAB:
-        case XEN_DOMCTL_PFINFO_L2TAB | XEN_DOMCTL_PFINFO_LPINTAB:
-
-        case XEN_DOMCTL_PFINFO_L3TAB:
-        case XEN_DOMCTL_PFINFO_L3TAB | XEN_DOMCTL_PFINFO_LPINTAB:
-
-        case XEN_DOMCTL_PFINFO_L4TAB:
-        case XEN_DOMCTL_PFINFO_L4TAB | XEN_DOMCTL_PFINFO_LPINTAB:
-
+        if ( page_type_has_stream_data(types[i]) )
             mfns[nr_pages++] = ctx->restore.ops.pfn_to_gfn(ctx, pfns[i]);
-            break;
-        }
     }
 
     /* Nothing to do? */
@@ -269,14 +252,8 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
 
     for ( i = 0, j = 0; i < count; ++i )
     {
-        switch ( types[i] )
-        {
-        case XEN_DOMCTL_PFINFO_XTAB:
-        case XEN_DOMCTL_PFINFO_BROKEN:
-        case XEN_DOMCTL_PFINFO_XALLOC:
-            /* No page data to deal with. */
+        if ( !page_type_has_stream_data(types[i]) )
             continue;
-        }
 
         if ( map_errs[j] )
         {
@@ -411,7 +388,7 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
             goto err;
         }
 
-        if ( type < XEN_DOMCTL_PFINFO_BROKEN )
+        if ( page_type_has_stream_data(type) )
             /* NOTAB and all L1 through L4 tables (including pinned) should
              * have a page worth of data in the record. */
             pages_of_data++;
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index c84a073e86f4..3fce41b6cfff 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -153,13 +153,8 @@ static int write_batch(struct xc_sr_context *ctx)
             goto err;
         }
 
-        switch ( types[i] )
-        {
-        case XEN_DOMCTL_PFINFO_BROKEN:
-        case XEN_DOMCTL_PFINFO_XALLOC:
-        case XEN_DOMCTL_PFINFO_XTAB:
+        if ( !page_type_has_stream_data(types[i]) )
             continue;
-        }
 
         mfns[nr_pages++] = mfns[i];
     }
@@ -177,13 +172,8 @@ static int write_batch(struct xc_sr_context *ctx)
 
         for ( i = 0, p = 0; i < nr_pfns; ++i )
         {
-            switch ( types[i] )
-            {
-            case XEN_DOMCTL_PFINFO_BROKEN:
-            case XEN_DOMCTL_PFINFO_XALLOC:
-            case XEN_DOMCTL_PFINFO_XTAB:
+            if ( !page_type_has_stream_data(types[i]) )
                 continue;
-            }
 
             if ( errors[p] )
             {
-- 
2.11.0

Re: [PATCH 1/5] tools/python: handle libxl__physmap_info.name properly in convert-legacy-stream

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

On Mon, Jul 05, 2021 at 08:10:23PM +0100, Andrew Cooper wrote:
> From: Olaf Hering <olaf@aepfle.de>
> 
> The trailing member name[] in libxl__physmap_info is written as a
> cstring into the stream. The current code does a sanity check if the
> last byte is zero. This attempt fails with python3 because name[-1]
> returns a type int. As a result the comparison with byte(\00) fails:
> 
>   File "/usr/lib/xen/bin/convert-legacy-stream", line 347, in read_libxl_toolstack
>     raise StreamError("physmap name not NUL terminated")
>   StreamError: physmap name not NUL terminated
> 
> To handle both python variants, cast to bytearray().
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
>  tools/python/scripts/convert-legacy-stream | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
> index 66ee3d2f5d29..227e1b5c3fd3 100755
> --- a/tools/python/scripts/convert-legacy-stream
> +++ b/tools/python/scripts/convert-legacy-stream
> @@ -343,7 +343,7 @@ def read_libxl_toolstack(vm, data):
>          if twidth == 64:
>              name = name[:-4]
>  
> -        if name[-1] != b'\x00':
> +        if bytearray(name)[-1] != 0:
>              raise StreamError("physmap name not NUL terminated")
>  
>          root = b"physmap/%x" % (phys, )
> -- 
> 2.11.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/5] tools/python: fix Python3.4 TypeError in format string

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Mon, Jul 05, 2021 at 08:10:24PM +0100, Andrew Cooper wrote:
> From: Olaf Hering <olaf@aepfle.de>
> 
> Using the first element of a tuple for a format specifier fails with
> python3.4 as included in SLE12:
>     b = b"string/%x" % (i, )
> TypeError: unsupported operand type(s) for %: 'bytes' and 'tuple'
> 
> It happens to work with python 2.7 and 3.6.
> To support older Py3, format as strings and explicitly encode as ASCII.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
>  tools/python/scripts/convert-legacy-stream | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
> index 227e1b5c3fd3..7fe375a66810 100755
> --- a/tools/python/scripts/convert-legacy-stream
> +++ b/tools/python/scripts/convert-legacy-stream
> @@ -346,9 +346,9 @@ def read_libxl_toolstack(vm, data):
>          if bytearray(name)[-1] != 0:
>              raise StreamError("physmap name not NUL terminated")
>  
> -        root = b"physmap/%x" % (phys, )
> -        kv = [root + b"/start_addr", b"%x" % (start, ),
> -              root + b"/size",       b"%x" % (size, ),
> +        root = ("physmap/%x" % (phys, )).encode('ascii')
> +        kv = [root + b"/start_addr", ("%x" % (start, )).encode('ascii'),
> +              root + b"/size",       ("%x" % (size, )).encode('ascii'),
>                root + b"/name",       name[:-1]]
>  
>          for key, val in zip(kv[0::2], kv[1::2]):
> -- 
> 2.11.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] tools/migration: unify type checking for data pfns in migration stream

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 393 bytes --]

On 05.07.21 21:10, Andrew Cooper wrote:
> From: Olaf Hering <olaf@aepfle.de>
> 
> Introduce a helper which decides if a given pfn type has data
> in the migration stream.
> 
> No change in behaviour intended, except for invalid page types which now
> have a safer default.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 0/5] tools/migration: Py3 fixes, and page type helpers

[Olaf Hering]

Am Mon, 5 Jul 2021 20:10:22 +0100
schrieb Andrew Cooper <andrew.cooper3@citrix.com>:

>   tools/python: handle libxl__physmap_info.name properly in convert-legacy-stream
>   tools/python: fix Python3.4 TypeError in format string
>   tools/migration: unify known page type checking
>   tools/migration: unify type checking for data pfns in the VM
>   tools/migration: unify type checking for data pfns in migration stream

Tested-by: Olaf Hering <olaf@aepfle.de>


Olaf