* [PATCH 0/2] hvm/save: cleanup
@ 2017-05-31 7:19 Jan Beulich
2017-05-31 7:24 ` [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jan Beulich @ 2017-05-31 7:19 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
1: sanitize DOMCTL_gethvmcontext_partial handling
2: clean up hvm_save_one()
Signed-off-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling
2017-05-31 7:19 [PATCH 0/2] hvm/save: cleanup Jan Beulich
@ 2017-05-31 7:24 ` Jan Beulich
2017-05-31 12:11 ` Wei Liu
2017-05-31 7:25 ` [PATCH 2/2] HVM: clean up hvm_save_one() Jan Beulich
2017-06-06 15:49 ` Ping: [PATCH 0/2] hvm/save: cleanup Jan Beulich
2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2017-05-31 7:24 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 4465 bytes --]
Have the caller indicate its buffer size, provide a means to query the
needed size, don't ignore the upper halves of type code and instance,
and don't copy partial data.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -496,6 +496,7 @@ int xc_domain_hvm_getcontext_partial(xc_
domctl.domain = (domid_t) domid;
domctl.u.hvmcontext_partial.type = typecode;
domctl.u.hvmcontext_partial.instance = instance;
+ domctl.u.hvmcontext_partial.bufsz = size;
set_xen_guest_handle(domctl.u.hvmcontext_partial.buffer, ctxt_buf);
ret = do_domctl(xch, &domctl);
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -590,8 +590,12 @@ long arch_do_domctl(
domain_pause(d);
ret = hvm_save_one(d, domctl->u.hvmcontext_partial.type,
domctl->u.hvmcontext_partial.instance,
- domctl->u.hvmcontext_partial.buffer);
+ domctl->u.hvmcontext_partial.buffer,
+ &domctl->u.hvmcontext_partial.bufsz);
domain_unpause(d);
+
+ if ( !ret )
+ copyback = true;
break;
case XEN_DOMCTL_set_address_size:
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -76,8 +76,8 @@ size_t hvm_save_size(struct domain *d)
/* Extract a single instance of a save record, by marshalling all
* records of that type and copying out the one we need. */
-int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
- XEN_GUEST_HANDLE_64(uint8) handle)
+int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
+ XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
{
int rv = -ENOENT;
size_t sz = 0;
@@ -117,16 +117,20 @@ int hvm_save_one(struct domain *d, uint1
desc = (void *)(ctxt.data + off);
/* Move past header */
off += sizeof(*desc);
+ if ( ctxt.cur < desc->length ||
+ off > ctxt.cur - desc->length )
+ break;
if ( instance == desc->instance )
{
- uint32_t copy_length = desc->length;
-
- if ( ctxt.cur < copy_length ||
- off > ctxt.cur - copy_length )
- copy_length = ctxt.cur - off;
rv = 0;
- if ( copy_to_guest(handle, ctxt.data + off, copy_length) )
+ if ( guest_handle_is_null(handle) )
+ *bufsz = desc->length;
+ else if ( *bufsz < desc->length )
+ rv = -ENOBUFS;
+ else if ( copy_to_guest(handle, ctxt.data + off, desc->length) )
rv = -EFAULT;
+ else
+ *bufsz = desc->length;
break;
}
}
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -37,7 +37,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000d
+#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000e
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -747,6 +747,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_debug
typedef struct xen_domctl_hvmcontext_partial {
uint32_t type; /* IN: Type of record required */
uint32_t instance; /* IN: Instance of that type */
+ uint64_aligned_t bufsz; /* IN: size of buffer */
XEN_GUEST_HANDLE_64(uint8) buffer; /* OUT: buffer to write record into */
} xen_domctl_hvmcontext_partial_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_hvmcontext_partial_t);
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -132,8 +132,8 @@ __initcall(__hvm_register_##_x##_save_an
/* Entry points for saving and restoring HVM domain state */
size_t hvm_save_size(struct domain *d);
int hvm_save(struct domain *d, hvm_domain_context_t *h);
-int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
- XEN_GUEST_HANDLE_64(uint8) handle);
+int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
+ XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz);
int hvm_load(struct domain *d, hvm_domain_context_t *h);
/* Arch-specific definitions. */
[-- Attachment #2: HVM-save-one-inputs.patch --]
[-- Type: text/plain, Size: 4516 bytes --]
HVM: sanitize DOMCTL_gethvmcontext_partial handling
Have the caller indicate its buffer size, provide a means to query the
needed size, don't ignore the upper halves of type code and instance,
and don't copy partial data.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -496,6 +496,7 @@ int xc_domain_hvm_getcontext_partial(xc_
domctl.domain = (domid_t) domid;
domctl.u.hvmcontext_partial.type = typecode;
domctl.u.hvmcontext_partial.instance = instance;
+ domctl.u.hvmcontext_partial.bufsz = size;
set_xen_guest_handle(domctl.u.hvmcontext_partial.buffer, ctxt_buf);
ret = do_domctl(xch, &domctl);
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -590,8 +590,12 @@ long arch_do_domctl(
domain_pause(d);
ret = hvm_save_one(d, domctl->u.hvmcontext_partial.type,
domctl->u.hvmcontext_partial.instance,
- domctl->u.hvmcontext_partial.buffer);
+ domctl->u.hvmcontext_partial.buffer,
+ &domctl->u.hvmcontext_partial.bufsz);
domain_unpause(d);
+
+ if ( !ret )
+ copyback = true;
break;
case XEN_DOMCTL_set_address_size:
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -76,8 +76,8 @@ size_t hvm_save_size(struct domain *d)
/* Extract a single instance of a save record, by marshalling all
* records of that type and copying out the one we need. */
-int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
- XEN_GUEST_HANDLE_64(uint8) handle)
+int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
+ XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
{
int rv = -ENOENT;
size_t sz = 0;
@@ -117,16 +117,20 @@ int hvm_save_one(struct domain *d, uint1
desc = (void *)(ctxt.data + off);
/* Move past header */
off += sizeof(*desc);
+ if ( ctxt.cur < desc->length ||
+ off > ctxt.cur - desc->length )
+ break;
if ( instance == desc->instance )
{
- uint32_t copy_length = desc->length;
-
- if ( ctxt.cur < copy_length ||
- off > ctxt.cur - copy_length )
- copy_length = ctxt.cur - off;
rv = 0;
- if ( copy_to_guest(handle, ctxt.data + off, copy_length) )
+ if ( guest_handle_is_null(handle) )
+ *bufsz = desc->length;
+ else if ( *bufsz < desc->length )
+ rv = -ENOBUFS;
+ else if ( copy_to_guest(handle, ctxt.data + off, desc->length) )
rv = -EFAULT;
+ else
+ *bufsz = desc->length;
break;
}
}
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -37,7 +37,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000d
+#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000e
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -747,6 +747,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_debug
typedef struct xen_domctl_hvmcontext_partial {
uint32_t type; /* IN: Type of record required */
uint32_t instance; /* IN: Instance of that type */
+ uint64_aligned_t bufsz; /* IN: size of buffer */
XEN_GUEST_HANDLE_64(uint8) buffer; /* OUT: buffer to write record into */
} xen_domctl_hvmcontext_partial_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_hvmcontext_partial_t);
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -132,8 +132,8 @@ __initcall(__hvm_register_##_x##_save_an
/* Entry points for saving and restoring HVM domain state */
size_t hvm_save_size(struct domain *d);
int hvm_save(struct domain *d, hvm_domain_context_t *h);
-int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
- XEN_GUEST_HANDLE_64(uint8) handle);
+int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
+ XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz);
int hvm_load(struct domain *d, hvm_domain_context_t *h);
/* Arch-specific definitions. */
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] HVM: clean up hvm_save_one()
2017-05-31 7:19 [PATCH 0/2] hvm/save: cleanup Jan Beulich
2017-05-31 7:24 ` [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling Jan Beulich
@ 2017-05-31 7:25 ` Jan Beulich
2017-06-06 17:52 ` Wei Liu
2017-06-06 15:49 ` Ping: [PATCH 0/2] hvm/save: cleanup Jan Beulich
2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2017-05-31 7:25 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]
Eliminate the for_each_vcpu() loop and the associated local variables,
don't override the save handler's return code, and correct formatting.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
{
- int rv = -ENOENT;
- size_t sz = 0;
- struct vcpu *v;
- hvm_domain_context_t ctxt = { 0, };
+ int rv;
+ hvm_domain_context_t ctxt = { };
const struct hvm_save_descriptor *desc;
- if ( d->is_dying
- || typecode > HVM_SAVE_CODE_MAX
- || hvm_sr_handlers[typecode].size < sizeof(*desc)
- || hvm_sr_handlers[typecode].save == NULL )
+ if ( d->is_dying ||
+ typecode > HVM_SAVE_CODE_MAX ||
+ hvm_sr_handlers[typecode].size < sizeof(*desc) ||
+ !hvm_sr_handlers[typecode].save )
return -EINVAL;
+ ctxt.size = hvm_sr_handlers[typecode].size;
if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
- for_each_vcpu(d, v)
- sz += hvm_sr_handlers[typecode].size;
- else
- sz = hvm_sr_handlers[typecode].size;
-
- ctxt.size = sz;
- ctxt.data = xmalloc_bytes(sz);
+ hvm_sr_handlers[typecode].size *= d->max_vcpus;
+ ctxt.data = xmalloc_bytes(ctxt.size);
if ( !ctxt.data )
return -ENOMEM;
- if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
- {
- printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
- d->domain_id, typecode);
- rv = -EFAULT;
- }
- else if ( ctxt.cur >= sizeof(*desc) )
+ if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
+ printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
+ d->domain_id, typecode, rv);
+ else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
{
uint32_t off;
[-- Attachment #2: HVM-save-one-cleanup.patch --]
[-- Type: text/plain, Size: 2126 bytes --]
HVM: clean up hvm_save_one()
Eliminate the for_each_vcpu() loop and the associated local variables,
don't override the save handler's return code, and correct formatting.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
{
- int rv = -ENOENT;
- size_t sz = 0;
- struct vcpu *v;
- hvm_domain_context_t ctxt = { 0, };
+ int rv;
+ hvm_domain_context_t ctxt = { };
const struct hvm_save_descriptor *desc;
- if ( d->is_dying
- || typecode > HVM_SAVE_CODE_MAX
- || hvm_sr_handlers[typecode].size < sizeof(*desc)
- || hvm_sr_handlers[typecode].save == NULL )
+ if ( d->is_dying ||
+ typecode > HVM_SAVE_CODE_MAX ||
+ hvm_sr_handlers[typecode].size < sizeof(*desc) ||
+ !hvm_sr_handlers[typecode].save )
return -EINVAL;
+ ctxt.size = hvm_sr_handlers[typecode].size;
if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
- for_each_vcpu(d, v)
- sz += hvm_sr_handlers[typecode].size;
- else
- sz = hvm_sr_handlers[typecode].size;
-
- ctxt.size = sz;
- ctxt.data = xmalloc_bytes(sz);
+ hvm_sr_handlers[typecode].size *= d->max_vcpus;
+ ctxt.data = xmalloc_bytes(ctxt.size);
if ( !ctxt.data )
return -ENOMEM;
- if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
- {
- printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
- d->domain_id, typecode);
- rv = -EFAULT;
- }
- else if ( ctxt.cur >= sizeof(*desc) )
+ if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
+ printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
+ d->domain_id, typecode, rv);
+ else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
{
uint32_t off;
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling
2017-05-31 7:24 ` [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling Jan Beulich
@ 2017-05-31 12:11 ` Wei Liu
0 siblings, 0 replies; 9+ messages in thread
From: Wei Liu @ 2017-05-31 12:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
On Wed, May 31, 2017 at 01:24:52AM -0600, Jan Beulich wrote:
> Have the caller indicate its buffer size, provide a means to query the
> needed size, don't ignore the upper halves of type code and instance,
> and don't copy partial data.
>
> Signed-off-by: Jan Beulich <jbeulich@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Ping: [PATCH 0/2] hvm/save: cleanup
2017-05-31 7:19 [PATCH 0/2] hvm/save: cleanup Jan Beulich
2017-05-31 7:24 ` [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling Jan Beulich
2017-05-31 7:25 ` [PATCH 2/2] HVM: clean up hvm_save_one() Jan Beulich
@ 2017-06-06 15:49 ` Jan Beulich
2 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2017-06-06 15:49 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson,
Stefano Stabellini, Konrad Rzeszutek Wilk, Tim Deegan
Cc: xen-devel
>>> On 31.05.17 at 09:19, <JBeulich@suse.com> wrote:
> 1: sanitize DOMCTL_gethvmcontext_partial handling
> 2: clean up hvm_save_one()
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] HVM: clean up hvm_save_one()
2017-05-31 7:25 ` [PATCH 2/2] HVM: clean up hvm_save_one() Jan Beulich
@ 2017-06-06 17:52 ` Wei Liu
2017-06-07 10:07 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2017-06-06 17:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote:
> Eliminate the for_each_vcpu() loop and the associated local variables,
> don't override the save handler's return code, and correct formatting.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/hvm/save.c
> +++ b/xen/common/hvm/save.c
> @@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
> int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
> XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
> {
> - int rv = -ENOENT;
> - size_t sz = 0;
> - struct vcpu *v;
> - hvm_domain_context_t ctxt = { 0, };
> + int rv;
> + hvm_domain_context_t ctxt = { };
> const struct hvm_save_descriptor *desc;
>
> - if ( d->is_dying
> - || typecode > HVM_SAVE_CODE_MAX
> - || hvm_sr_handlers[typecode].size < sizeof(*desc)
> - || hvm_sr_handlers[typecode].save == NULL )
> + if ( d->is_dying ||
> + typecode > HVM_SAVE_CODE_MAX ||
> + hvm_sr_handlers[typecode].size < sizeof(*desc) ||
> + !hvm_sr_handlers[typecode].save )
> return -EINVAL;
>
> + ctxt.size = hvm_sr_handlers[typecode].size;
> if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
> - for_each_vcpu(d, v)
> - sz += hvm_sr_handlers[typecode].size;
> - else
> - sz = hvm_sr_handlers[typecode].size;
> -
> - ctxt.size = sz;
> - ctxt.data = xmalloc_bytes(sz);
> + hvm_sr_handlers[typecode].size *= d->max_vcpus;
Why is size updated with a particular d->max_vcpus here? AFAICT (after
going through layers of macros ...) hvm_sr_handlers is global and needed
when saving any hvm guests. The "size" field contains the length of one
record.
Also, you set ctxt.size before this loop without taking into account the
number of vcpus, which looks wrong to me. Shouldn't it be (when not
updating hvm_sr_handlers[typecode].size)
ctxt.size = hvm_sr_handlers[typecode].size * d->max_vcpus
?
> + ctxt.data = xmalloc_bytes(ctxt.size);
> if ( !ctxt.data )
> return -ENOMEM;
>
> - if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
> - {
> - printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
> - d->domain_id, typecode);
> - rv = -EFAULT;
> - }
> - else if ( ctxt.cur >= sizeof(*desc) )
> + if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
> + printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
> + d->domain_id, typecode, rv);
> + else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
I guess the intent here is to set rv while at the same time only test
ctxt.cur? But why? Can the code be reorganised so that it is easier to
reason about.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] HVM: clean up hvm_save_one()
2017-06-06 17:52 ` Wei Liu
@ 2017-06-07 10:07 ` Jan Beulich
2017-06-07 10:29 ` Wei Liu
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2017-06-07 10:07 UTC (permalink / raw)
To: Wei Liu
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 06.06.17 at 19:52, <wei.liu2@citrix.com> wrote:
> On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote:
>> Eliminate the for_each_vcpu() loop and the associated local variables,
>> don't override the save handler's return code, and correct formatting.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/common/hvm/save.c
>> +++ b/xen/common/hvm/save.c
>> @@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
>> int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int
> instance,
>> XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
>> {
>> - int rv = -ENOENT;
>> - size_t sz = 0;
>> - struct vcpu *v;
>> - hvm_domain_context_t ctxt = { 0, };
>> + int rv;
>> + hvm_domain_context_t ctxt = { };
>> const struct hvm_save_descriptor *desc;
>>
>> - if ( d->is_dying
>> - || typecode > HVM_SAVE_CODE_MAX
>> - || hvm_sr_handlers[typecode].size < sizeof(*desc)
>> - || hvm_sr_handlers[typecode].save == NULL )
>> + if ( d->is_dying ||
>> + typecode > HVM_SAVE_CODE_MAX ||
>> + hvm_sr_handlers[typecode].size < sizeof(*desc) ||
>> + !hvm_sr_handlers[typecode].save )
>> return -EINVAL;
>>
>> + ctxt.size = hvm_sr_handlers[typecode].size;
>> if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
>> - for_each_vcpu(d, v)
>> - sz += hvm_sr_handlers[typecode].size;
>> - else
>> - sz = hvm_sr_handlers[typecode].size;
>> -
>> - ctxt.size = sz;
>> - ctxt.data = xmalloc_bytes(sz);
>> + hvm_sr_handlers[typecode].size *= d->max_vcpus;
>
> Why is size updated with a particular d->max_vcpus here? AFAICT (after
> going through layers of macros ...) hvm_sr_handlers is global and needed
> when saving any hvm guests. The "size" field contains the length of one
> record.
>
> Also, you set ctxt.size before this loop without taking into account the
> number of vcpus, which looks wrong to me. Shouldn't it be (when not
> updating hvm_sr_handlers[typecode].size)
>
> ctxt.size = hvm_sr_handlers[typecode].size * d->max_vcpus
>
> ?
Right, this is complete rubbish. Should be
ctxt.size *= d->max_vcpus;
>> + ctxt.data = xmalloc_bytes(ctxt.size);
>> if ( !ctxt.data )
>> return -ENOMEM;
>>
>> - if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
>> - {
>> - printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
>> - d->domain_id, typecode);
>> - rv = -EFAULT;
>> - }
>> - else if ( ctxt.cur >= sizeof(*desc) )
>> + if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
>> + printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
>> + d->domain_id, typecode, rv);
>> + else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
>
> I guess the intent here is to set rv while at the same time only test
> ctxt.cur? But why?
Well, we can't use -ENOENT as initializer anymore, as rv now is
being modified above. Before entering the body of the "else if"
it needs to be -ENOENT though.
> Can the code be reorganised so that it is easier to reason about.
It probably could be, at the expense of assigning -ENOENT in two
places.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] HVM: clean up hvm_save_one()
2017-06-07 10:07 ` Jan Beulich
@ 2017-06-07 10:29 ` Wei Liu
2017-06-07 10:39 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2017-06-07 10:29 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
On Wed, Jun 07, 2017 at 04:07:02AM -0600, Jan Beulich wrote:
> >>> On 06.06.17 at 19:52, <wei.liu2@citrix.com> wrote:
> > On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote:
> >> Eliminate the for_each_vcpu() loop and the associated local variables,
> >> don't override the save handler's return code, and correct formatting.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/common/hvm/save.c
> >> +++ b/xen/common/hvm/save.c
> >> @@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
> >> int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int
> > instance,
> >> XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
> >> {
> >> - int rv = -ENOENT;
> >> - size_t sz = 0;
> >> - struct vcpu *v;
> >> - hvm_domain_context_t ctxt = { 0, };
> >> + int rv;
> >> + hvm_domain_context_t ctxt = { };
> >> const struct hvm_save_descriptor *desc;
> >>
> >> - if ( d->is_dying
> >> - || typecode > HVM_SAVE_CODE_MAX
> >> - || hvm_sr_handlers[typecode].size < sizeof(*desc)
> >> - || hvm_sr_handlers[typecode].save == NULL )
> >> + if ( d->is_dying ||
> >> + typecode > HVM_SAVE_CODE_MAX ||
> >> + hvm_sr_handlers[typecode].size < sizeof(*desc) ||
> >> + !hvm_sr_handlers[typecode].save )
> >> return -EINVAL;
> >>
> >> + ctxt.size = hvm_sr_handlers[typecode].size;
> >> if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
> >> - for_each_vcpu(d, v)
> >> - sz += hvm_sr_handlers[typecode].size;
> >> - else
> >> - sz = hvm_sr_handlers[typecode].size;
> >> -
> >> - ctxt.size = sz;
> >> - ctxt.data = xmalloc_bytes(sz);
> >> + hvm_sr_handlers[typecode].size *= d->max_vcpus;
> >
> > Why is size updated with a particular d->max_vcpus here? AFAICT (after
> > going through layers of macros ...) hvm_sr_handlers is global and needed
> > when saving any hvm guests. The "size" field contains the length of one
> > record.
> >
> > Also, you set ctxt.size before this loop without taking into account the
> > number of vcpus, which looks wrong to me. Shouldn't it be (when not
> > updating hvm_sr_handlers[typecode].size)
> >
> > ctxt.size = hvm_sr_handlers[typecode].size * d->max_vcpus
> >
> > ?
>
> Right, this is complete rubbish. Should be
>
> ctxt.size *= d->max_vcpus;
>
Yes, this looks right to me now.
> >> + ctxt.data = xmalloc_bytes(ctxt.size);
> >> if ( !ctxt.data )
> >> return -ENOMEM;
> >>
> >> - if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
> >> - {
> >> - printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
> >> - d->domain_id, typecode);
> >> - rv = -EFAULT;
> >> - }
> >> - else if ( ctxt.cur >= sizeof(*desc) )
> >> + if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
> >> + printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
> >> + d->domain_id, typecode, rv);
> >> + else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
> >
> > I guess the intent here is to set rv while at the same time only test
> > ctxt.cur? But why?
>
> Well, we can't use -ENOENT as initializer anymore, as rv now is
> being modified above. Before entering the body of the "else if"
> it needs to be -ENOENT though.
>
> > Can the code be reorganised so that it is easier to reason about.
>
> It probably could be, at the expense of assigning -ENOENT in two
> places.
>
How about:
if ( (rv = hvm_sr_handlers ...)) != 0 )
{
} else {
rv = -ENOENT;
if ( ctx.cur >= sizeof(*desc) )
{
}
}
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] HVM: clean up hvm_save_one()
2017-06-07 10:29 ` Wei Liu
@ 2017-06-07 10:39 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2017-06-07 10:39 UTC (permalink / raw)
To: Wei Liu
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 07.06.17 at 12:29, <wei.liu2@citrix.com> wrote:
> On Wed, Jun 07, 2017 at 04:07:02AM -0600, Jan Beulich wrote:
>> >>> On 06.06.17 at 19:52, <wei.liu2@citrix.com> wrote:
>> > On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote:
>> >> if ( !ctxt.data )
>> >> return -ENOMEM;
>> >>
>> >> - if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
>> >> - {
>> >> - printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
>> >> - d->domain_id, typecode);
>> >> - rv = -EFAULT;
>> >> - }
>> >> - else if ( ctxt.cur >= sizeof(*desc) )
>> >> + if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
>> >> + printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
>> >> + d->domain_id, typecode, rv);
>> >> + else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
>> >
>> > I guess the intent here is to set rv while at the same time only test
>> > ctxt.cur? But why?
>>
>> Well, we can't use -ENOENT as initializer anymore, as rv now is
>> being modified above. Before entering the body of the "else if"
>> it needs to be -ENOENT though.
>>
>> > Can the code be reorganised so that it is easier to reason about.
>>
>> It probably could be, at the expense of assigning -ENOENT in two
>> places.
>>
>
> How about:
>
> if ( (rv = hvm_sr_handlers ...)) != 0 )
> {
> } else {
> rv = -ENOENT;
>
> if ( ctx.cur >= sizeof(*desc) )
> {
>
> }
>
> }
Well, that would require re-indenting the entire body, which I
wanted to avoid as well.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-07 10:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-31 7:19 [PATCH 0/2] hvm/save: cleanup Jan Beulich
2017-05-31 7:24 ` [PATCH 1/2] HVM: sanitize DOMCTL_gethvmcontext_partial handling Jan Beulich
2017-05-31 12:11 ` Wei Liu
2017-05-31 7:25 ` [PATCH 2/2] HVM: clean up hvm_save_one() Jan Beulich
2017-06-06 17:52 ` Wei Liu
2017-06-07 10:07 ` Jan Beulich
2017-06-07 10:29 ` Wei Liu
2017-06-07 10:39 ` Jan Beulich
2017-06-06 15:49 ` Ping: [PATCH 0/2] hvm/save: cleanup Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).