[PATCH RFC 0/4] Fix issues with zero-length records in migration v2

[PATCH RFC 0/4] Fix issues with zero-length records in migration v2

[Andrew Cooper]

This series is RFC because it has only had compile testing thusfar.

On AMD hardware supporting Debug Extentions, migration of a PV guest which is
not currently using Debug Extentions fails, as the save side writes a
X86_PV_VCPU_MSRS record with 0 content, which the receving side chokes on.

It was alway the intention that such a record would be omitted, but that
obviously didn't go as intended.

Adjust the docs to clarify that such records should be omitted, but that
receving sides should tolerate their presence.

Andrew Cooper (4):
  docs: Clarify the expected behaviour of zero length records
  tools/libxc: Tolerate zero-length records in migration v2 streams
  tools/libxc: Avoid generating inappropriate zero-length records
  tools/python: Adjust migration v2 library to warn about zero-length
    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 1/4] docs: Clarify the expected behaviour of zero length records

[Andrew Cooper]

The sending side shouldn't send any variable sized 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>
---
 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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
+depending on their content.  A record with variable size which ends up being
+zero should be omitted entirely from the stream by the sending side.
+
 x86 PV Guest
 ------------
 
@@ -719,3 +723,13 @@ 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
+======
+
+For compatibility with older code, the receving side of a stream should
+tolerate and ignore variable sized records with zero content.  Some releases
+of Xen 4.6 and 4.7 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

[PATCH 2/4] tools/libxc: Tolerate zero-length records in migration v2 streams

[Andrew Cooper]

Under some circumstances, the migration v2 save code would generate valid
records with zero content, when the intended behaviour was to omit the record
entirely.

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

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 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 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Andrew Cooper]

It was never intended for records such as these to be sent with zero content.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.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 ba50a43..5401bf9 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 4a29460..5fb9f2f 100644
--- a/tools/libxc/xc_sr_save_x86_pv.c
+++ b/tools/libxc/xc_sr_save_x86_pv.c
@@ -607,6 +607,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);
 }
@@ -662,6 +666,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;
@@ -728,6 +736,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 4/4] tools/python: Adjust migration v2 library to warn about zero-length 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>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.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 85a78f4..2037c85 100644
--- a/tools/python/xen/migration/libxc.py
+++ b/tools/python/xen/migration/libxc.py
@@ -308,6 +308,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), ))
@@ -320,10 +324,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:
@@ -381,6 +389,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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[Wei Liu]

On Thu, Jul 21, 2016 at 06:17:34PM +0100, Andrew Cooper wrote:
> The sending side shouldn't send any variable sized 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>

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 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Wei Liu]

On Thu, Jul 21, 2016 at 06:17:36PM +0100, Andrew Cooper wrote:
> It was never intended for records such as these to be sent with zero content.
> 

Wouldn't it be better to modify write_split_record to ignore zero
content instead of patching up different places?

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.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 ba50a43..5401bf9 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 4a29460..5fb9f2f 100644
> --- a/tools/libxc/xc_sr_save_x86_pv.c
> +++ b/tools/libxc/xc_sr_save_x86_pv.c
> @@ -607,6 +607,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);
>  }
> @@ -662,6 +666,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;
> @@ -728,6 +736,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

Re: [PATCH 2/4] tools/libxc: Tolerate zero-length records in migration v2 streams

[Wei Liu]

On Thu, Jul 21, 2016 at 06:17:35PM +0100, Andrew Cooper wrote:
> Under some circumstances, the migration v2 save code would generate valid
> records with zero content, when the intended behaviour was to omit the record
> entirely.
> 
> 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>

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

Re: [PATCH 4/4] tools/python: Adjust migration v2 library to warn about zero-length records

[Wei Liu]

On Thu, Jul 21, 2016 at 06:17:37PM +0100, Andrew Cooper wrote:
> 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>

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Andrew Cooper]

On 25/07/16 10:45, Wei Liu wrote:
> On Thu, Jul 21, 2016 at 06:17:36PM +0100, Andrew Cooper wrote:
>> It was never intended for records such as these to be sent with zero content.
>>
> Wouldn't it be better to modify write_split_record to ignore zero
> content instead of patching up different places?

That would catch the HVM_PARAMS, but not the other 3, which have an
extra 8 byte header.

However, having write_split_record() conditionally drop records is a
violation of the principle of least surprise.  Choosing to skip a record
is an important decision and needs to be easy to spot in the code.

~Andrew

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Wei Liu]

On Mon, Jul 25, 2016 at 10:57:13AM +0100, Andrew Cooper wrote:
> On 25/07/16 10:45, Wei Liu wrote:
> > On Thu, Jul 21, 2016 at 06:17:36PM +0100, Andrew Cooper wrote:
> >> It was never intended for records such as these to be sent with zero content.
> >>
> > Wouldn't it be better to modify write_split_record to ignore zero
> > content instead of patching up different places?
> 
> That would catch the HVM_PARAMS, but not the other 3, which have an
> extra 8 byte header.
> 
> However, having write_split_record() conditionally drop records is a
> violation of the principle of least surprise.  Choosing to skip a record
> is an important decision and needs to be easy to spot in the code.
> 

Fair enough.

> ~Andrew

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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[David Vrabel]

On 21/07/16 18:17, Andrew Cooper wrote:
> The sending side shouldn't send any variable sized 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>
> ---
>  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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
> +depending on their content.  A record with variable size which ends up being
> +zero should be omitted entirely from the stream by the sending side.

I disagree. I think the stream should include the records with the empty
content.  This gives better consistency and does not require changes to
the stream.

David

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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[Andrew Cooper]

On 25/07/16 11:21, David Vrabel wrote:
> On 21/07/16 18:17, Andrew Cooper wrote:
>> The sending side shouldn't send any variable sized 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>
>> ---
>>  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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
>> +depending on their content.  A record with variable size which ends up being
>> +zero should be omitted entirely from the stream by the sending side.
> I disagree. I think the stream should include the records with the empty
> content.  This gives better consistency and does not require changes to
> the stream.

There are already some which are properly omitted, like the vcpu records
for offline vcpus.

There is no point having empty records; omitting them is an optimisation
which we absolutely shouldn't preclude.

~Andrew

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[David Vrabel]

On 21/07/16 18:17, Andrew Cooper wrote:
> It was never intended for records such as these to be sent with zero content.

As the original author of the specification I'm perhaps best placed to
say what the original intention is.

For records such as HVM_PARAMS which consist of a set of N items, the
intention was to most definitely send a record with 0 items.

For records that fetch an opaque blob from the hypervisor, again the
intention was to sent this blob as-is with no sort of processing or
other checking. i.e., if the hypervisor gives us a zero-length blob we
sent that as-is.

This makes all the streams look the same with all the same records,
regardless of what hardware platform it was run on.  Including
zero-length/count records also makes diagnosing problems easier -- the
empty record is visible in the stream instead of having to remember that
sometimes these records are deliberately omitted.

As such, this series should be limited to making the restore side handle
the zero count sets or zero length blobs if it does not do so already.

The specification should be clarified to note that some records may have
zero-length blobs or contain zero items.

David


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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[David Vrabel]

On 25/07/16 11:25, Andrew Cooper wrote:
> On 25/07/16 11:21, David Vrabel wrote:
>> On 21/07/16 18:17, Andrew Cooper wrote:
>>> The sending side shouldn't send any variable sized 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>
>>> ---
>>>  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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
>>> +depending on their content.  A record with variable size which ends up being
>>> +zero should be omitted entirely from the stream by the sending side.
>> I disagree. I think the stream should include the records with the empty
>> content.  This gives better consistency and does not require changes to
>> the stream.
> 
> There are already some which are properly omitted, like the vcpu records
> for offline vcpus.
> 
> There is no point having empty records; omitting them is an optimisation
> which we absolutely shouldn't preclude.

The optimization doesn't matter since these records are so tiny.

I've expanded on why these should be included in another reply.

David

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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[Andrew Cooper]

On 25/07/16 11:35, David Vrabel wrote:
> On 25/07/16 11:25, Andrew Cooper wrote:
>> On 25/07/16 11:21, David Vrabel wrote:
>>> On 21/07/16 18:17, Andrew Cooper wrote:
>>>> The sending side shouldn't send any variable sized 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>
>>>> ---
>>>>  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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
>>>> +depending on their content.  A record with variable size which ends up being
>>>> +zero should be omitted entirely from the stream by the sending side.
>>> I disagree. I think the stream should include the records with the empty
>>> content.  This gives better consistency and does not require changes to
>>> the stream.
>> There are already some which are properly omitted, like the vcpu records
>> for offline vcpus.
>>
>> There is no point having empty records; omitting them is an optimisation
>> which we absolutely shouldn't preclude.
> The optimization doesn't matter since these records are so tiny.
>
> I've expanded on why these should be included in another reply.

We already omit most zero-length records.  That ship has already sailed.

This bug only presents itself because of two hypercalls in Xen returning
different size characteristics.  We already checked for zero first, and
fail to check for zero the second time.

~Andrew

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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[David Vrabel]

On 25/07/16 11:38, Andrew Cooper wrote:
> On 25/07/16 11:35, David Vrabel wrote:
>> On 25/07/16 11:25, Andrew Cooper wrote:
>>> On 25/07/16 11:21, David Vrabel wrote:
>>>> On 21/07/16 18:17, Andrew Cooper wrote:
>>>>> The sending side shouldn't send any variable sized 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>
>>>>> ---
>>>>>  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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
>>>>> +depending on their content.  A record with variable size which ends up being
>>>>> +zero should be omitted entirely from the stream by the sending side.
>>>> I disagree. I think the stream should include the records with the empty
>>>> content.  This gives better consistency and does not require changes to
>>>> the stream.
>>> There are already some which are properly omitted, like the vcpu records
>>> for offline vcpus.
>>>
>>> There is no point having empty records; omitting them is an optimisation
>>> which we absolutely shouldn't preclude.
>> The optimization doesn't matter since these records are so tiny.
>>
>> I've expanded on why these should be included in another reply.
> 
> We already omit most zero-length records.  That ship has already sailed.
> 
> This bug only presents itself because of two hypercalls in Xen returning
> different size characteristics.  We already checked for zero first, and
> fail to check for zero the second time.

Then document that those specific records may be omitted, but don't
spread the brokenness to other (future) records.

David


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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[Andrew Cooper]

On 25/07/16 11:44, David Vrabel wrote:
> On 25/07/16 11:38, Andrew Cooper wrote:
>> On 25/07/16 11:35, David Vrabel wrote:
>>> On 25/07/16 11:25, Andrew Cooper wrote:
>>>> On 25/07/16 11:21, David Vrabel wrote:
>>>>> On 21/07/16 18:17, Andrew Cooper wrote:
>>>>>> The sending side shouldn't send any variable sized 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>
>>>>>> ---
>>>>>>  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..a90bc5d 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,10 @@ 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 have an exactly specified size.  Some records have variable size
>>>>>> +depending on their content.  A record with variable size which ends up being
>>>>>> +zero should be omitted entirely from the stream by the sending side.
>>>>> I disagree. I think the stream should include the records with the empty
>>>>> content.  This gives better consistency and does not require changes to
>>>>> the stream.
>>>> There are already some which are properly omitted, like the vcpu records
>>>> for offline vcpus.
>>>>
>>>> There is no point having empty records; omitting them is an optimisation
>>>> which we absolutely shouldn't preclude.
>>> The optimization doesn't matter since these records are so tiny.
>>>
>>> I've expanded on why these should be included in another reply.
>> We already omit most zero-length records.  That ship has already sailed.
>>
>> This bug only presents itself because of two hypercalls in Xen returning
>> different size characteristics.  We already checked for zero first, and
>> fail to check for zero the second time.
> Then document that those specific records may be omitted, but don't
> spread the brokenness to other (future) records.

I disagree, and do not consider this broken.

~Andrew

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

Re: [PATCH 1/4] docs: Clarify the expected behaviour of zero length records

[Ian Jackson]

Andrew Cooper writes ("[PATCH 1/4] docs: Clarify the expected behaviour of zero length records"):
> The sending side shouldn't send any variable sized records which end
> up having zero content, but the receiving side will need to tolerate
> such records for compatibility purposes.
...
> +Some records have an exactly specified size.  Some records have
> +variable size depending on their content.  A record with variable
> +size which ends up being zero should be omitted entirely from the
> +stream by the sending side.

I'm not sure this is best stated as a general rule.

Consider a record type which specifies `explicit wombat layout' and
consists of a list of `wombat locations'.

Absence of the record would be semantically different from an empty
record.  In the absence of the record, the receiver would do automatic
choice of number and layout of wombats.  With an empty record, the
receiver would create zero wombats (which might be different to the
default).


I think the underlying spec bug here is that the specification for
`X86_PV_VCPU_BASIC, EXTENDED, XSAVE, MSRS' is too vague.  It simply
specifies
  context          Binary data for this VCPU.
and `The format of these records are identical.  They are all binary
blobs of data which are accessed using specific pairs of domctl
hypercalls.'.


I wasn't able to immediately find the corresponding hypercalls (which
is another docs bug).

But if the hypercalls generate and consume 0-length blocks, then I
think it is fine for the migration stream to contain them.


Have I misunderstood ?

Ian.

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Ian Jackson]

David Vrabel writes ("Re: [Xen-devel] [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records"):
> For records such as HVM_PARAMS which consist of a set of N items, the
> intention was to most definitely send a record with 0 items.
> 
> For records that fetch an opaque blob from the hypervisor, again the
> intention was to sent this blob as-is with no sort of processing or
> other checking. i.e., if the hypervisor gives us a zero-length blob we
> sent that as-is.
> 
> This makes all the streams look the same with all the same records,
> regardless of what hardware platform it was run on.  Including
> zero-length/count records also makes diagnosing problems easier -- the
> empty record is visible in the stream instead of having to remember that
> sometimes these records are deliberately omitted.
> 
> As such, this series should be limited to making the restore side handle
> the zero count sets or zero length blobs if it does not do so already.
> 
> The specification should be clarified to note that some records may have
> zero-length blobs or contain zero items.

I think I prefer David's view here, but I don't quite feel I
understand what the underlying bug is.

Ian.

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

Re: [PATCH 2/4] tools/libxc: Tolerate zero-length records in migration v2 streams

[David Vrabel]

On 21/07/16 18:17, Andrew Cooper wrote:
> Under some circumstances, the migration v2 save code would generate valid
> records with zero content, when the intended behaviour was to omit the record
                                      ^^^^^^^^
As explained, this is not the intended behaviour.  I would appreciate it
if you did not misrepresent me here.

> entirely.
> 
> As the stream is otherwise fine, tolerate these records and avoid failing the
> migration.
[...]
> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
[...]
> +    /*
> +     * 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++ )

This loop already handles hdr->count == 0.  The additional check is not
required.

> --- 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;
> +    }

This check for a zero-sized blob should be immediately prior to the

   blob = malloc(blobsz);

So all the other length validation is not skipped.  In particular, some
record types may wish to make zero-length blobs invalid.

David

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

Re: [PATCH 2/4] tools/libxc: Tolerate zero-length records in migration v2 streams

[Andrew Cooper]

On 25/07/16 13:21, David Vrabel wrote:
> On 21/07/16 18:17, Andrew Cooper wrote:
>> Under some circumstances, the migration v2 save code would generate valid
>> records with zero content, when the intended behaviour was to omit the record
>                                       ^^^^^^^^
> As explained, this is not the intended behaviour.

And as previously stated, I still disagree.

>   I would appreciate it
> if you did not misrepresent me here.

WTF?

I am the author of this code, and I very definitely intended for the
record to be omitted.  I am not representing you in at all, let alone
mis-representation.

There is clearly a difference of opinion, and there is no guidance from
the spec, but accusations like this are not warranted.

~Andrew

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

Re: [PATCH 2/4] tools/libxc: Tolerate zero-length records in migration v2 streams

[David Vrabel]

On 25/07/16 13:46, Andrew Cooper wrote:
> On 25/07/16 13:21, David Vrabel wrote:
>> On 21/07/16 18:17, Andrew Cooper wrote:
>>> Under some circumstances, the migration v2 save code would generate valid
>>> records with zero content, when the intended behaviour was to omit the record
>>                                       ^^^^^^^^
>> As explained, this is not the intended behaviour.
> 
> And as previously stated, I still disagree.
> 
>>   I would appreciate it
>> if you did not misrepresent me here.
> 
> WTF?
>
> I am the author of this code, and I very definitely intended for the
> record to be omitted.  I am not representing you in at all, let alone
> mis-representation.

As the original and principle author of the specification I definitely
intended for empty records to be included.  In the context above (given
the lack of clarity in the text) it is not unreasonable for someone to
conclude that its talking about the specification author's intentions.

Thus without clarifying whose intentions you're talking about, you are
mis-representing my profession opinion.

So, yes. I am serious about you changing the commit message. I do not
want your design decisions (good or bad) attributed to me.

David

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Ian Jackson]

David Vrabel writes ("Re: [Xen-devel] [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records"):
> On 21/07/16 18:17, Andrew Cooper wrote:
> > It was never intended for records such as these to be sent with zero content.
> 
> As the original author of the specification I'm perhaps best placed to
> say what the original intention is.

I think this discussion of `the intent' is not particularly helpful,
when the authors of the spec document, and of the code, disagree.  It
is in any case not necessary to decide what `the original intent' is
or was.  Accordingly, can all participants please stop referring to
`the intent' in this way.  (It is of course fine to write `_my_
intent'.)

What is necessary is to decide what should be done now.

I am going to quote liberally from the rest of David's mail but adjust
some of the wording to try to make it something I can agree with:

  For records such as HVM_PARAMS which consist of a set of N items,
  David's intention in the spec, was to most definitely send a record
  with 0 items.

  For records that fetch an opaque blob from the hypervisor, again,
  David's intention was to send this blob as-is with no sort of
  processing or other checking. i.e., if the hypervisor gives us a
  zero-length blob we sent that as-is.

  This makes all the streams look the same with all the same records,
  regardless of what hardware platform it was run on.  Including
  zero-length/count records also makes diagnosing problems easier -- the
  empty record is visible in the stream instead of having to remember that
  sometimes these records are deliberately omitted.

  As such, IMO this series should be limited to making the restore
  side handle the zero count sets or zero length blobs if it does not
  do so already.

  The specification should be clarified to note that some records may have
  zero-length blobs or contain zero items.

I reviewed the spec in detail at the time and I agree with David's
point of view as I have rephrased (hopefully without annoying David)
above.

I see no reason why zero-content-length records should be treated as
any kind of special case.

Is the ultimate bug that we are tripping over here simply that the
code calls malloc(0) and then bails if the libc produces NULL (as it
is entitled to do) ?

Thanks,
Ian.

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Wei Liu]

On Mon, Jul 25, 2016 at 06:15:37PM +0100, Ian Jackson wrote:
> David Vrabel writes ("Re: [Xen-devel] [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records"):
> > On 21/07/16 18:17, Andrew Cooper wrote:
> > > It was never intended for records such as these to be sent with zero content.
> > 
> > As the original author of the specification I'm perhaps best placed to
> > say what the original intention is.
> 
> I think this discussion of `the intent' is not particularly helpful,
> when the authors of the spec document, and of the code, disagree.  It
> is in any case not necessary to decide what `the original intent' is
> or was.  Accordingly, can all participants please stop referring to
> `the intent' in this way.  (It is of course fine to write `_my_
> intent'.)
> 
> What is necessary is to decide what should be done now.
> 
> I am going to quote liberally from the rest of David's mail but adjust
> some of the wording to try to make it something I can agree with:
> 
>   For records such as HVM_PARAMS which consist of a set of N items,
>   David's intention in the spec, was to most definitely send a record
>   with 0 items.
> 
>   For records that fetch an opaque blob from the hypervisor, again,
>   David's intention was to send this blob as-is with no sort of
>   processing or other checking. i.e., if the hypervisor gives us a
>   zero-length blob we sent that as-is.
> 
>   This makes all the streams look the same with all the same records,
>   regardless of what hardware platform it was run on.  Including
>   zero-length/count records also makes diagnosing problems easier -- the
>   empty record is visible in the stream instead of having to remember that
>   sometimes these records are deliberately omitted.
> 
>   As such, IMO this series should be limited to making the restore
>   side handle the zero count sets or zero length blobs if it does not
>   do so already.
> 
>   The specification should be clarified to note that some records may have
>   zero-length blobs or contain zero items.
> 
> I reviewed the spec in detail at the time and I agree with David's
> point of view as I have rephrased (hopefully without annoying David)
> above.
> 
> I see no reason why zero-content-length records should be treated as
> any kind of special case.
> 
> Is the ultimate bug that we are tripping over here simply that the
> code calls malloc(0) and then bails if the libc produces NULL (as it
> is entitled to do) ?
> 

No, it isn't.

AIUI the issue is receiving end can't deal with zero-length record.  To
to more precise, it is the hypervisor that chokes when toolstack issues
an hypercall with the "malformed" data.

Hence the two approaches presented: one is to omit zero-length record,
the other is to tolerate zero-length record.

If we go with David's approach, I think hypervisor should be made
tolerant to zero-length record. That would be symmetric on both ends --
hv can spit out as well as accept zero-length records. Toolstack should
transparently send and receive records.

Wei.


> Thanks,
> Ian.

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

Re: [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records"):
> On Mon, Jul 25, 2016 at 06:15:37PM +0100, Ian Jackson wrote:
> > Is the ultimate bug that we are tripping over here simply that the
> > code calls malloc(0) and then bails if the libc produces NULL (as it
> > is entitled to do) ?
> 
> No, it isn't.
> 
> AIUI the issue is receiving end can't deal with zero-length record.  To
> to more precise, it is the hypervisor that chokes when toolstack issues
> an hypercall with the "malformed" data.

Oh.  So the hypervisor produces this zero-length data, but rejects it
when the same zero-length data is supplied back to it ?  That seems
like a bug in the hypervisor to me.

> If we go with David's approach, I think hypervisor should be made
> tolerant to zero-length record. That would be symmetric on both ends --
> hv can spit out as well as accept zero-length records. Toolstack should
> transparently send and receive records.

That would seem sensible to me.

Ian.

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

Re: [PATCH RFC 0/4] Fix issues with zero-length records in migration v2

[Julien Grall]

Hi Andrew,

On 21/07/16 18:17, Andrew Cooper wrote:
> This series is RFC because it has only had compile testing thusfar.
>
> On AMD hardware supporting Debug Extentions, migration of a PV guest which is
> not currently using Debug Extentions fails, as the save side writes a
> X86_PV_VCPU_MSRS record with 0 content, which the receving side chokes on.
>
> It was alway the intention that such a record would be omitted, but that
> obviously didn't go as intended.
>
> Adjust the docs to clarify that such records should be omitted, but that
> receving sides should tolerate their presence.

Last December you requested this series to be a blocker for Xen 4.9 (see 
[1]). Do you have any update on the status?

Cheers,

[1] https://marc.info/?l=xen-devel&m=148251247319180

-- 
Julien Grall

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

Re: [PATCH RFC 0/4] Fix issues with zero-length records in migration v2

[Andrew Cooper]

On 14/03/17 13:20, Julien Grall wrote:
> Hi Andrew,
>
> On 21/07/16 18:17, Andrew Cooper wrote:
>> This series is RFC because it has only had compile testing thusfar.
>>
>> On AMD hardware supporting Debug Extentions, migration of a PV guest
>> which is
>> not currently using Debug Extentions fails, as the save side writes a
>> X86_PV_VCPU_MSRS record with 0 content, which the receving side
>> chokes on.
>>
>> It was alway the intention that such a record would be omitted, but that
>> obviously didn't go as intended.
>>
>> Adjust the docs to clarify that such records should be omitted, but that
>> receving sides should tolerate their presence.
>
> Last December you requested this series to be a blocker for Xen 4.9
> (see [1]). Do you have any update on the status?

As previously identified, my opinion is that the code as presented in v1
is correct, and there have been no reasonable arguments to the contrary.

I can post v1 again, but it won't be altered.

~Andrew

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

Re: [PATCH RFC 0/4] Fix issues with zero-length records in migration v2

[Wei Liu]

On Tue, Mar 14, 2017 at 01:50:44PM +0000, Andrew Cooper wrote:
> On 14/03/17 13:20, Julien Grall wrote:
> > Hi Andrew,
> >
> > On 21/07/16 18:17, Andrew Cooper wrote:
> >> This series is RFC because it has only had compile testing thusfar.
> >>
> >> On AMD hardware supporting Debug Extentions, migration of a PV guest
> >> which is
> >> not currently using Debug Extentions fails, as the save side writes a
> >> X86_PV_VCPU_MSRS record with 0 content, which the receving side
> >> chokes on.
> >>
> >> It was alway the intention that such a record would be omitted, but that
> >> obviously didn't go as intended.
> >>
> >> Adjust the docs to clarify that such records should be omitted, but that
> >> receving sides should tolerate their presence.
> >
> > Last December you requested this series to be a blocker for Xen 4.9
> > (see [1]). Do you have any update on the status?
> 
> As previously identified, my opinion is that the code as presented in v1
> is correct, and there have been no reasonable arguments to the contrary.
> 
> I can post v1 again, but it won't be altered.
> 

What about the subthread starting from
<22422.18745.380486.234849@mariner.uk.xensource.com> ?

> ~Andrew

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

Re: [PATCH RFC 0/4] Fix issues with zero-length records in migration v2

[Julien Grall]

Hi all,

On 14/03/17 14:21, Wei Liu wrote:
> On Tue, Mar 14, 2017 at 01:50:44PM +0000, Andrew Cooper wrote:
>> On 14/03/17 13:20, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 21/07/16 18:17, Andrew Cooper wrote:
>>>> This series is RFC because it has only had compile testing thusfar.
>>>>
>>>> On AMD hardware supporting Debug Extentions, migration of a PV guest
>>>> which is
>>>> not currently using Debug Extentions fails, as the save side writes a
>>>> X86_PV_VCPU_MSRS record with 0 content, which the receving side
>>>> chokes on.
>>>>
>>>> It was alway the intention that such a record would be omitted, but that
>>>> obviously didn't go as intended.
>>>>
>>>> Adjust the docs to clarify that such records should be omitted, but that
>>>> receving sides should tolerate their presence.
>>>
>>> Last December you requested this series to be a blocker for Xen 4.9
>>> (see [1]). Do you have any update on the status?
>>
>> As previously identified, my opinion is that the code as presented in v1
>> is correct, and there have been no reasonable arguments to the contrary.
>>
>> I can post v1 again, but it won't be altered.
>>
>
> What about the subthread starting from
> <22422.18745.380486.234849@mariner.uk.xensource.com> ?

Gentle ping on this subject. Any consensus on the way to fix it?

Cheers,

-- 
Julien Grall

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