[PATCH] libxc: correct error message in xc_sr_common.c

[PATCH] libxc: correct error message in xc_sr_common.c

[Juergen Gross]

When the record length for sending the p2m frames in a migration
stream is too large, the issued error message is not very helpful:

xc: Record (0x00000003, x86 PV P2M frames) length 0x8 exceeds max
    (0x800000): Internal error

When printing the error use the size which was tested instead that of
the record header length.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_sr_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index 48fa676f4e..79b9c3e940 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -71,8 +71,8 @@ int write_split_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
 
     if ( record_length > REC_LENGTH_MAX )
     {
-        ERROR("Record (0x%08x, %s) length %#x exceeds max (%#x)", rec->type,
-              rec_type_to_str(rec->type), rec->length, REC_LENGTH_MAX);
+        ERROR("Record (0x%08x, %s) length %#zx exceeds max (%#x)", rec->type,
+              rec_type_to_str(rec->type), record_length, REC_LENGTH_MAX);
         return -1;
     }
 
-- 
2.12.3


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

[PATCH] libxc: increase maximum migration stream record length

[Juergen Gross]

Today the maximum record lenth in a migration stream is 8MB. This
limits the size of a PV domain to a little bit less than 1TB in the
migration case, as the P2M frame list will exceed 8MB in this case.

Raising the record size limit by a factor of 16 allows for domain
sizes of nearly 16TB to be migrated. This ought to be enough.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_sr_stream_format.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_sr_stream_format.h b/tools/libxc/xc_sr_stream_format.h
index 3291b256fd..15ff1c7efb 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -57,8 +57,8 @@ struct xc_sr_rhdr
 
 /* All records must be aligned up to an 8 octet boundary */
 #define REC_ALIGN_ORDER               (3U)
-/* Somewhat arbitrary - 8MB */
-#define REC_LENGTH_MAX                (8U << 20)
+/* Somewhat arbitrary - 128MB */
+#define REC_LENGTH_MAX                (128U << 20)
 
 #define REC_TYPE_END                        0x00000000U
 #define REC_TYPE_PAGE_DATA                  0x00000001U
-- 
2.12.3


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

Re: [PATCH] libxc: correct error message in xc_sr_common.c

[Andrew Cooper]

On 10/08/17 12:24, Juergen Gross wrote:
> When the record length for sending the p2m frames in a migration
> stream is too large, the issued error message is not very helpful:
>
> xc: Record (0x00000003, x86 PV P2M frames) length 0x8 exceeds max
>     (0x800000): Internal error
>
> When printing the error use the size which was tested instead that of
> the record header length.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is a side effect of how the function was restructured during
original development.

> ---
>  tools/libxc/xc_sr_common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
> index 48fa676f4e..79b9c3e940 100644
> --- a/tools/libxc/xc_sr_common.c
> +++ b/tools/libxc/xc_sr_common.c
> @@ -71,8 +71,8 @@ int write_split_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
>  
>      if ( record_length > REC_LENGTH_MAX )
>      {
> -        ERROR("Record (0x%08x, %s) length %#x exceeds max (%#x)", rec->type,
> -              rec_type_to_str(rec->type), rec->length, REC_LENGTH_MAX);
> +        ERROR("Record (0x%08x, %s) length %#zx exceeds max (%#x)", rec->type,
> +              rec_type_to_str(rec->type), record_length, REC_LENGTH_MAX);
>          return -1;
>      }
>  


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

Re: [PATCH] libxc: increase maximum migration stream record length

[Andrew Cooper]

On 10/08/17 12:24, Juergen Gross wrote:
> Today the maximum record lenth in a migration stream is 8MB. This
> limits the size of a PV domain to a little bit less than 1TB in the
> migration case, as the P2M frame list will exceed 8MB in this case.
>
> Raising the record size limit by a factor of 16 allows for domain
> sizes of nearly 16TB to be migrated. This ought to be enough.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Hmm - Changing this isn't something I've considered when it comes to ABI
compatibility.  I also see that there is no mention of the maximum
record length in the stream spec, which is an oversight.

Worse still, there is no record length check in the python utilities,
but both sides of the C code perform the check.

Let me ponder the implications.

~Andrew

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

Re: [PATCH] libxc: correct error message in xc_sr_common.c

[Wei Liu]

On Thu, Aug 10, 2017 at 01:24:27PM +0200, Juergen Gross wrote:
> When the record length for sending the p2m frames in a migration
> stream is too large, the issued error message is not very helpful:
> 
> xc: Record (0x00000003, x86 PV P2M frames) length 0x8 exceeds max
>     (0x800000): Internal error
> 
> When printing the error use the size which was tested instead that of
> the record header length.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

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

Re: [PATCH] libxc: increase maximum migration stream record length

[Juergen Gross]

On 10/08/17 14:26, Andrew Cooper wrote:
> On 10/08/17 12:24, Juergen Gross wrote:
>> Today the maximum record lenth in a migration stream is 8MB. This
>> limits the size of a PV domain to a little bit less than 1TB in the
>> migration case, as the P2M frame list will exceed 8MB in this case.
>>
>> Raising the record size limit by a factor of 16 allows for domain
>> sizes of nearly 16TB to be migrated. This ought to be enough.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Hmm - Changing this isn't something I've considered when it comes to ABI
> compatibility.  I also see that there is no mention of the maximum
> record length in the stream spec, which is an oversight.
> 
> Worse still, there is no record length check in the python utilities,
> but both sides of the C code perform the check.
> 
> Let me ponder the implications.

Any result yet?


Juergen

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

Re: [PATCH] libxc: increase maximum migration stream record length

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] libxc: increase maximum migration stream record length"):
> On 10/08/17 12:24, Juergen Gross wrote:
> > Today the maximum record lenth in a migration stream is 8MB. This
> > limits the size of a PV domain to a little bit less than 1TB in the
> > migration case, as the P2M frame list will exceed 8MB in this case.
> >
> > Raising the record size limit by a factor of 16 allows for domain
> > sizes of nearly 16TB to be migrated. This ought to be enough.
> >
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Hmm - Changing this isn't something I've considered when it comes to ABI
> compatibility.  I also see that there is no mention of the maximum
> record length in the stream spec, which is an oversight.
> 
> Worse still, there is no record length check in the python utilities,
> but both sides of the C code perform the check.
> 
> Let me ponder the implications.

I think simply changing this #define is the right approach.

What we mostly care about is that old senders can successfully send
data to new receivers, for which this is not an issue.

As regards new senders and old receivers:

This #define is not used to actually control the size of outgoing
records.  The only mentions are in safety checks, in both sending and
receiving side.

Therefore, in a situation where the old code would generate a
particular stream without error, the new code would generate exactly
the same stream.  (Likewise, obviously, the interpretation of existing
valid streams is not changed.)

The only difference in behaviour is that in some situations the old
sender will throw an error and abandon the migration attempt.  In
these same situations the new sender will generate a stream which will
be rejected by old receivers, but accepted by new receivers.

So increasing this #define is good:

 * All existing workin use cases work as before.

 * The new use case works with new code at both ends
   (this is the best that can be done because AIUI there is
    no way to represent this domain in a way that the old code
    would understand - although I have not verified this).

 * In one of the non-working cases the error handling is changed:
   the error is now detected at the receiver rather than at the
   sender.  This is, however, fine.

So:

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

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

Re: [PATCH] libxc: increase maximum migration stream record length

[Wei Liu]

On Thu, Aug 31, 2017 at 11:24:23AM +0100, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH] libxc: increase maximum migration stream record length"):
> > On 10/08/17 12:24, Juergen Gross wrote:
> > > Today the maximum record lenth in a migration stream is 8MB. This
> > > limits the size of a PV domain to a little bit less than 1TB in the
> > > migration case, as the P2M frame list will exceed 8MB in this case.
> > >
> > > Raising the record size limit by a factor of 16 allows for domain
> > > sizes of nearly 16TB to be migrated. This ought to be enough.
> > >
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Hmm - Changing this isn't something I've considered when it comes to ABI
> > compatibility.  I also see that there is no mention of the maximum
> > record length in the stream spec, which is an oversight.
> > 
> > Worse still, there is no record length check in the python utilities,
> > but both sides of the C code perform the check.
> > 
> > Let me ponder the implications.
> 
> I think simply changing this #define is the right approach.
> 
> What we mostly care about is that old senders can successfully send
> data to new receivers, for which this is not an issue.
> 
> As regards new senders and old receivers:
> 
> This #define is not used to actually control the size of outgoing
> records.  The only mentions are in safety checks, in both sending and
> receiving side.
> 
> Therefore, in a situation where the old code would generate a
> particular stream without error, the new code would generate exactly
> the same stream.  (Likewise, obviously, the interpretation of existing
> valid streams is not changed.)
> 
> The only difference in behaviour is that in some situations the old
> sender will throw an error and abandon the migration attempt.  In
> these same situations the new sender will generate a stream which will
> be rejected by old receivers, but accepted by new receivers.
> 
> So increasing this #define is good:
> 
>  * All existing workin use cases work as before.
> 
>  * The new use case works with new code at both ends
>    (this is the best that can be done because AIUI there is
>     no way to represent this domain in a way that the old code
>     would understand - although I have not verified this).
> 
>  * In one of the non-working cases the error handling is changed:
>    the error is now detected at the receiver rather than at the
>    sender.  This is, however, fine.
> 
> So:
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied.

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