Re: [Xen-devel] [PATCH v17 11/13] x86/domctl: Use hvm_save_vcpu_handler

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Aug 22, 2018 at 05:02:41PM +0300, Alexandru Isaila wrote:
> This patch is aimed on using the new save_one fuctions in the hvm_save
> 
> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> 
> ---
> Changes since V15:
>       - Moved declarations into their scopes
>       - Remove redundant NULL check
>       - Remove rc variable
>       - Change fault return to -ENODATA.
> ---
>  xen/arch/x86/hvm/save.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> index 1106b96..1eb2b01 100644
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -195,7 +195,6 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
>      char *c;
>      struct hvm_save_header hdr;
>      struct hvm_save_end end;
> -    hvm_save_handler handler;
>      unsigned int i;
>  
>      if ( d->is_dying )
> @@ -223,17 +222,37 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
>      /* Save all available kinds of state */
>      for ( i = 0; i <= HVM_SAVE_CODE_MAX; i++ )
>      {
> -        handler = hvm_sr_handlers[i].save;
> -        if ( handler != NULL )
> +        struct vcpu *v;

This could be moved...

> +        hvm_save_vcpu_handler save_one_handler = hvm_sr_handlers[i].save_one;
> +        hvm_save_handler handler = hvm_sr_handlers[i].save;;

Double ';'.

Also, I would expect that a device either has a global save handler
(handler) or a per-vcpu save handler (save_one_handler), but not both
at the same time?

In which case, I would add something like:

ASSERT(save_one_handler == NULL || handler == NULL);

In order to make sure both are not set at the same time.

> +
> +        if ( save_one_handler )
> +        {

...here if you are reducing the scope.

> +            for_each_vcpu ( d, v )
> +            {
> +                printk(XENLOG_G_INFO "HVM %pv save: %s\n",
> +                       v, hvm_sr_handlers[i].name);
> +
> +                if ( save_one_handler(v, h) != 0 )
> +                {
> +                    printk(XENLOG_G_ERR
> +                           "HVM %pv save: failed to save type %"PRIu16"\n",

I wonder why PRIu16 is used here in order to print i which is unsigned
int. Isn't it the same as using '%u'?

> +                           v, i);
> +                    return -ENODATA;
> +                }
> +            }
> +        }
> +        else if ( handler )
>          {
>              printk(XENLOG_G_INFO "HVM%d save: %s\n",
>                     d->domain_id, hvm_sr_handlers[i].name);
> +

Stray newline?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel