[PATCH for-4.9 0/4] Fix migration of PV guests on modern AMD hardware

[PATCH for-4.9 0/4] Fix migration of PV guests on modern AMD hardware

[Andrew Cooper]

This series fixes https://xenproject.atlassian.net/browse/XEN-5

Andrew Cooper (4):
  tools/libxc: Tolerate specific zero-content records in migration v2 streams
  tools/libxc: Avoid generating inappropriate zero-content records
  tools/python: Adjust migration v2 library to warn about zero-content records
  docs: Clarify the expected behaviour of zero-content records

 docs/specs/libxc-migration-stream.pandoc | 16 +++++++++++++++-
 tools/libxc/xc_sr_restore_x86_hvm.c      | 25 ++++++++++++++++++++++---
 tools/libxc/xc_sr_restore_x86_pv.c       | 17 ++++++++++++++---
 tools/libxc/xc_sr_save_x86_hvm.c         |  4 ++++
 tools/libxc/xc_sr_save_x86_pv.c          | 12 ++++++++++++
 tools/python/xen/migration/libxc.py      | 13 ++++++++++++-
 6 files changed, 79 insertions(+), 8 deletions(-)

-- 
2.1.4


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

[PATCH for-4.9 1/4] tools/libxc: Tolerate specific zero-content records in migration v2 streams

[Andrew Cooper]

The migration v2 save code was written to avoid sending data records with no
content, as such records serve no purpose but come with a performance hit.
The restore code sanity checks this expectation.

Under some circumstances (most notably, on AMD hardware with Debug Extensions,
and a PV guest kernel which is not using the feature), the save code would
generate a record with no content, which trips the sanity check in the restore
code.

As the stream is otherwise fine, tolerate these records and avoid failing the
migration.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Julien Grall <julien.grall@arm.com>

This needs backporting to Xen 4.6, and fixes XEN-5
---
 tools/libxc/xc_sr_restore_x86_hvm.c | 25 ++++++++++++++++++++++---
 tools/libxc/xc_sr_restore_x86_pv.c  | 17 ++++++++++++++---
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 49d22c7..1dca853 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -39,13 +39,32 @@ static int handle_hvm_params(struct xc_sr_context *ctx,
     unsigned int i;
     int rc;
 
-    if ( rec->length < sizeof(*hdr)
-         || rec->length < sizeof(*hdr) + hdr->count * sizeof(*entry) )
+    if ( rec->length < sizeof(*hdr) )
     {
-        ERROR("hvm_params record is too short");
+        ERROR("HVM_PARAMS record truncated: length %u, header size %zu",
+              rec->length, sizeof(*hdr));
         return -1;
     }
 
+    if ( rec->length != (sizeof(*hdr) + hdr->count * sizeof(*entry)) )
+    {
+        ERROR("HVM_PARAMS record truncated: header %zu, count %u, "
+              "expected len %zu, got %u",
+              sizeof(*hdr), hdr->count, hdr->count * sizeof(*entry),
+              rec->length);
+        return -1;
+    }
+
+    /*
+     * Tolerate empty records.  Older sending sides used to accidentally
+     * generate them.
+     */
+    if ( hdr->count == 0 )
+    {
+        DBGPRINTF("Skipping empty HVM_PARAMS record\n");
+        return 0;
+    }
+
     for ( i = 0; i < hdr->count; i++, entry++ )
     {
         switch ( entry->index )
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index bc604b3..50e25c1 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -753,15 +753,26 @@ static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
     }
 
     /* Confirm that there is a complete header. */
-    if ( rec->length <= sizeof(*vhdr) )
+    if ( rec->length < sizeof(*vhdr) )
     {
-        ERROR("%s record truncated: length %u, min %zu",
-              rec_name, rec->length, sizeof(*vhdr) + 1);
+        ERROR("%s record truncated: length %u, header size %zu",
+              rec_name, rec->length, sizeof(*vhdr));
         goto out;
     }
 
     blobsz = rec->length - sizeof(*vhdr);
 
+    /*
+     * Tolerate empty records.  Older sending sides used to accidentally
+     * generate them.
+     */
+    if ( blobsz == 0 )
+    {
+        DBGPRINTF("Skipping empty %s record for vcpu %u\n",
+                  rec_type_to_str(rec->type), vhdr->vcpu_id);
+        goto out;
+    }
+
     /* Check that the vcpu id is within range. */
     if ( vhdr->vcpu_id >= ctx->x86_pv.restore.nr_vcpus )
     {
-- 
2.1.4


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

[PATCH for-4.9 2/4] tools/libxc: Avoid generating inappropriate zero-content records

[Andrew Cooper]

The code as written attempted to elide zero-content records, as such records
serve no purpose but come with a performance hit.  Unfortunately, in the case
where the hypervisor reported max size is non-zero, but the actual size is
zero, the record is not elided.

This previously tripped up the sanity checks in the restore side of migration,
but as the underlying reasons for eliding the records in the first place are
still valid, fix the elision logic.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/xc_sr_save_x86_hvm.c |  4 ++++
 tools/libxc/xc_sr_save_x86_pv.c  | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index e485928..fc5c6ea 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -112,6 +112,10 @@ static int write_hvm_params(struct xc_sr_context *ctx)
         }
     }
 
+    /* No params? Skip this record. */
+    if ( hdr.count == 0 )
+        return 0;
+
     rc = write_split_record(ctx, &rec, entries, hdr.count * sizeof(*entries));
     if ( rc )
         PERROR("Failed to write HVM_PARAMS record");
diff --git a/tools/libxc/xc_sr_save_x86_pv.c b/tools/libxc/xc_sr_save_x86_pv.c
index f218d17..36b1058 100644
--- a/tools/libxc/xc_sr_save_x86_pv.c
+++ b/tools/libxc/xc_sr_save_x86_pv.c
@@ -609,6 +609,10 @@ static int write_one_vcpu_extended(struct xc_sr_context *ctx, uint32_t id)
         return -1;
     }
 
+    /* No content? Skip the record. */
+    if ( domctl.u.ext_vcpucontext.size == 0 )
+        return 0;
+
     return write_split_record(ctx, &rec, &domctl.u.ext_vcpucontext,
                               domctl.u.ext_vcpucontext.size);
 }
@@ -664,6 +668,10 @@ static int write_one_vcpu_xsave(struct xc_sr_context *ctx, uint32_t id)
         goto err;
     }
 
+    /* No xsave state? Skip this record. */
+    if ( domctl.u.vcpuextstate.size == 0 )
+        goto out;
+
     rc = write_split_record(ctx, &rec, buffer, domctl.u.vcpuextstate.size);
     if ( rc )
         goto err;
@@ -730,6 +738,10 @@ static int write_one_vcpu_msrs(struct xc_sr_context *ctx, uint32_t id)
         goto err;
     }
 
+    /* No MSRs? Skip this record. */
+    if ( domctl.u.vcpu_msrs.msr_count == 0 )
+        goto out;
+
     rc = write_split_record(ctx, &rec, buffer,
                             domctl.u.vcpu_msrs.msr_count *
                             sizeof(xen_domctl_vcpu_msr_t));
-- 
2.1.4


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

[PATCH for-4.9 3/4] tools/python: Adjust migration v2 library to warn about zero-content records

[Andrew Cooper]

These records shouldn't be in a stream, but accidentally are.  Warn about
them, but don't abort the verification.

While here, add a missing length check to the X86_PV_P2M_FRAMES record
checker.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/python/xen/migration/libxc.py | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/python/xen/migration/libxc.py b/tools/python/xen/migration/libxc.py
index 6fd3f6d..f24448a 100644
--- a/tools/python/xen/migration/libxc.py
+++ b/tools/python/xen/migration/libxc.py
@@ -312,6 +312,10 @@ class VerifyLibxc(VerifyBase):
     def verify_record_x86_pv_p2m_frames(self, content):
         """ x86 PV p2m frames record """
 
+        if len(content) < 8:
+            raise RecordError("x86_pv_p2m_frames: record length must be at"
+                              " least 8 bytes long")
+
         if len(content) % 8 != 0:
             raise RecordError("Length expected to be a multiple of 8, not %d"
                               % (len(content), ))
@@ -324,10 +328,14 @@ class VerifyLibxc(VerifyBase):
         """ Generic for all REC_TYPE_x86_pv_vcpu_{basic,extended,xsave,msrs} """
         minsz = calcsize(X86_PV_VCPU_HDR_FORMAT)
 
-        if len(content) <= minsz:
+        if len(content) < minsz:
             raise RecordError("X86_PV_VCPU_%s record length must be at least %d"
                               " bytes long" % (name, minsz))
 
+        if len(content) == minsz:
+            self.info("Warning: X86_PV_VCPU_%s record with zero content"
+                      % (name, ))
+
         vcpuid, res1 = unpack(X86_PV_VCPU_HDR_FORMAT, content[:minsz])
 
         if res1 != 0:
@@ -385,6 +393,9 @@ class VerifyLibxc(VerifyBase):
         if rsvd != 0:
             raise RecordError("Reserved field not zero (0x%04x)" % (rsvd, ))
 
+        if count == 0:
+            self.info("Warning: HVM_PARAMS record with zero content")
+
         sz += count * calcsize(HVM_PARAMS_ENTRY_FORMAT)
 
         if len(content) != sz:
-- 
2.1.4


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

[PATCH for-4.9 4/4] docs: Clarify the expected behaviour of zero-content records

[Andrew Cooper]

The sending side shouldn't send any data records which end up having zero
content, but the receiving side will need to tolerate such records for
compatibility purposes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 docs/specs/libxc-migration-stream.pandoc | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 31eba10..546baab 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -3,7 +3,7 @@
   Andrew Cooper <<andrew.cooper3@citrix.com>>
   Wen Congyang <<wency@cn.fujitsu.com>>
   Yang Hongyang <<hongyang.yang@easystack.cn>>
-% Revision 1
+% Revision 2
 
 Introduction
 ============
@@ -631,6 +631,11 @@ The set of valid records depends on the guest architecture and type.  No
 assumptions should be made about the ordering or interleaving of
 independent records.  Record dependencies are noted below.
 
+Some records are used for signalling, and explicitly have zero length.  All
+other records contain data relevent to the migration.  Data records with no
+content should be elided on the source side, as they their presence serves no
+purpose, but result in extra work for the restore side.
+
 x86 PV Guest
 ------------
 
@@ -719,3 +724,12 @@ restored.
 The image header may only be extended by _appending_ additional
 fields.  In particular, the `marker`, `id` and `version` fields must
 never change size or location.
+
+
+Errata
+======
+
+1. For compatibility with older code, the receving side of a stream should
+   tolerate and ignore variable sized records with zero content.  Xen releases
+   between 4.6 and 4.8 could end up generating valid HVM\_PARAMS or
+   X86\_PV\_VCPU\_{EXTENDED,XSAVE,MSRS} records with 0 content.
-- 
2.1.4


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

Re: [PATCH for-4.9 2/4] tools/libxc: Avoid generating inappropriate zero-content records

[Jan Beulich]

>>> On 30.03.17 at 18:32, <andrew.cooper3@citrix.com> wrote:
> --- a/tools/libxc/xc_sr_save_x86_hvm.c
> +++ b/tools/libxc/xc_sr_save_x86_hvm.c
> @@ -112,6 +112,10 @@ static int write_hvm_params(struct xc_sr_context *ctx)
>          }
>      }
>  
> +    /* No params? Skip this record. */
> +    if ( hdr.count == 0 )
> +        return 0;

Purely out of curiosity - under what conditions would this happen?
Some of the params in the array look like they would always have
a non-zero value.

Jan


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

Re: [PATCH for-4.9 4/4] docs: Clarify the expected behaviour of zero-content records

[Jan Beulich]

>>> On 30.03.17 at 18:32, <andrew.cooper3@citrix.com> wrote:
> @@ -631,6 +631,11 @@ The set of valid records depends on the guest architecture and type.  No
>  assumptions should be made about the ordering or interleaving of
>  independent records.  Record dependencies are noted below.
>  
> +Some records are used for signalling, and explicitly have zero length.  All
> +other records contain data relevent to the migration.  Data records with no

relevant?

> +content should be elided on the source side, as they their presence serves no

Stray "they"?

> +purpose, but result in extra work for the restore side.

results?

> @@ -719,3 +724,12 @@ restored.
>  The image header may only be extended by _appending_ additional
>  fields.  In particular, the `marker`, `id` and `version` fields must
>  never change size or location.
> +
> +
> +Errata
> +======
> +
> +1. For compatibility with older code, the receving side of a stream should
> +   tolerate and ignore variable sized records with zero content.  Xen releases
> +   between 4.6 and 4.8 could end up generating valid HVM\_PARAMS or
> +   X86\_PV\_VCPU\_{EXTENDED,XSAVE,MSRS} records with 0 content.

Also elsewhere in the series you use expressions similar to this "0
content", but especially here (with no code next to it) it is rather
ambiguous: Do you mean zero-length content, or non-zero-length
content being all zero, or both?

Jan


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

Re: [PATCH for-4.9 0/4] Fix migration of PV guests on modern AMD hardware

[Julien Grall]

Hi Andrew,

Thank you for resending the series.

On 30/03/17 17:32, Andrew Cooper wrote:
> This series fixes https://xenproject.atlassian.net/browse/XEN-5

This is a blocker for Xen 4.9. Wei, Ian, would you have time to review 
this series?

Cheers,

>
> Andrew Cooper (4):
>   tools/libxc: Tolerate specific zero-content records in migration v2 streams
>   tools/libxc: Avoid generating inappropriate zero-content records
>   tools/python: Adjust migration v2 library to warn about zero-content records
>   docs: Clarify the expected behaviour of zero-content records
>
>  docs/specs/libxc-migration-stream.pandoc | 16 +++++++++++++++-
>  tools/libxc/xc_sr_restore_x86_hvm.c      | 25 ++++++++++++++++++++++---
>  tools/libxc/xc_sr_restore_x86_pv.c       | 17 ++++++++++++++---
>  tools/libxc/xc_sr_save_x86_hvm.c         |  4 ++++
>  tools/libxc/xc_sr_save_x86_pv.c          | 12 ++++++++++++
>  tools/python/xen/migration/libxc.py      | 13 ++++++++++++-
>  6 files changed, 79 insertions(+), 8 deletions(-)
>

-- 
Julien Grall

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

Re: [PATCH for-4.9 0/4] Fix migration of PV guests on modern AMD hardware

[Wei Liu]

On Wed, Apr 05, 2017 at 12:23:58PM +0100, Julien Grall wrote:
> Hi Andrew,
> 
> Thank you for resending the series.
> 
> On 30/03/17 17:32, Andrew Cooper wrote:
> > This series fixes https://xenproject.atlassian.net/browse/XEN-5
> 
> This is a blocker for Xen 4.9. Wei, Ian, would you have time to review this
> series?
> 

It's on my list. I will get to it later today.

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

Re: [PATCH for-4.9 2/4] tools/libxc: Avoid generating inappropriate zero-content records

[Wei Liu]

On Thu, Mar 30, 2017 at 05:32:32PM +0100, Andrew Cooper wrote:
> The code as written attempted to elide zero-content records, as such records
> serve no purpose but come with a performance hit.  Unfortunately, in the case
> where the hypervisor reported max size is non-zero, but the actual size is
> zero, the record is not elided.
> 
> This previously tripped up the sanity checks in the restore side of migration,
> but as the underlying reasons for eliding the records in the first place are
> still valid, fix the elision logic.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.9 4/4] docs: Clarify the expected behaviour of zero-content records

[Wei Liu]

On Thu, Mar 30, 2017 at 05:32:34PM +0100, Andrew Cooper wrote:
> The sending side shouldn't send any data records which end up having zero
> content, but the receiving side will need to tolerate such records for
> compatibility purposes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With the comments from Jan addressed:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.9 4/4] docs: Clarify the expected behaviour of zero-content records

[Wei Liu]

On Fri, Mar 31, 2017 at 01:51:09AM -0600, Jan Beulich wrote:
> >>> On 30.03.17 at 18:32, <andrew.cooper3@citrix.com> wrote:
> > @@ -631,6 +631,11 @@ The set of valid records depends on the guest architecture and type.  No
> >  assumptions should be made about the ordering or interleaving of
> >  independent records.  Record dependencies are noted below.
> >  
> > +Some records are used for signalling, and explicitly have zero length.  All
> > +other records contain data relevent to the migration.  Data records with no
> 
> relevant?
> 
> > +content should be elided on the source side, as they their presence serves no
> 
> Stray "they"?
> 
> > +purpose, but result in extra work for the restore side.
> 
> results?
> 
> > @@ -719,3 +724,12 @@ restored.
> >  The image header may only be extended by _appending_ additional
> >  fields.  In particular, the `marker`, `id` and `version` fields must
> >  never change size or location.
> > +
> > +
> > +Errata
> > +======
> > +
> > +1. For compatibility with older code, the receving side of a stream should
> > +   tolerate and ignore variable sized records with zero content.  Xen releases
> > +   between 4.6 and 4.8 could end up generating valid HVM\_PARAMS or
> > +   X86\_PV\_VCPU\_{EXTENDED,XSAVE,MSRS} records with 0 content.
> 
> Also elsewhere in the series you use expressions similar to this "0
> content", but especially here (with no code next to it) it is rather
> ambiguous: Do you mean zero-length content, or non-zero-length
> content being all zero, or both?
> 

IIRC it is the former, zero-length content. I will fix that up while
committing.

Andrew if you think that's wrong, submit a patch to fix it.

Wei.

> Jan
> 

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