Re: [Xen-devel] [PATCH v9 03/11] x86/hvm: Introduce hvm_save_cpu_ctxt_one func

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.06.18 at 11:25, <aisaila@xxxxxxxxxxxxxxx> wrote:
> +static int hvm_save_cpu_ctxt_one(struct vcpu *v, hvm_domain_context_t *h)
> +{
> +    struct segment_register seg;
> +    struct hvm_hw_cpu ctxt;
> +
> +    memset(&ctxt, 0, sizeof(ctxt));
> +
> +    /*
> +     * We don't need to save state for a vcpu that is down; the restore
> +     * code will leave it down if there is nothing saved.
> +     */
> +    if ( v->pause_flags & VPF_down )
> +        return CONTINUE;

Note how the original code had if() and memset() the other way around.

>  static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  {
>      struct vcpu *v;
> -    struct hvm_hw_cpu ctxt;
> -    struct segment_register seg;
> +    int rc = 0;
>  
>      for_each_vcpu ( d, v )
>      {
> -        /* We don't need to save state for a vcpu that is down; the restore 
> -         * code will leave it down if there is nothing saved. */
> -        if ( v->pause_flags & VPF_down )
> +        rc = hvm_save_cpu_ctxt_one(v, h);
> +        if (rc == CONTINUE)

Style. I'm pretty sure you were asked before to go through and
check your additions for style.

> --- a/xen/include/asm-x86/hvm/support.h
> +++ b/xen/include/asm-x86/hvm/support.h
> @@ -52,6 +52,8 @@ extern unsigned int opt_hvm_debug_level;
>  #define HVM_DBG_LOG(level, _f, _a...) do {} while (0)
>  #endif
>  
> +#define CONTINUE 2

This is way too generic an identifier name. And it's not helpful at
all without other possible values also enumerated. And please
take "enumerated" as a hint ... Otoh, looking at its use - this is
an agreement between hvm_save_cpu_ctxt() and
hvm_save_cpu_ctxt_one() only. Why does such need a globally
visible #define?

Jan



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