Re: [Xen-devel] [PATCH v15 09/14] x86/hvm: Introduce lapic_save_regs_one func

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.18 at 15:53, <aisaila@xxxxxxxxxxxxxxx> wrote:
> This is used to save data from a single instance.
> 
> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/vlapic.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 0795161..d35810e 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1460,26 +1460,37 @@ static int lapic_save_hidden(struct domain *d, 
> hvm_domain_context_t *h)
>      return err;
>  }
>  
> +static int lapic_save_regs_one(struct vcpu *v, hvm_domain_context_t *h)
> +{
> +    struct vlapic *s;
> +
> +    if ( !has_vlapic(v->domain) )
> +        return 0;
> +
> +    if ( hvm_funcs.sync_pir_to_irr )
> +        hvm_funcs.sync_pir_to_irr(v);
> +
> +    s = vcpu_vlapic(v);
> +
> +    return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs);
> +}

Here as well as in patch 8 there's little point in having a local variable s
which is used just once. If you really think you want to retain them,
here it can be pointer to const (other than in patch 8 afaict), and like
in patch 8 it could have an initializer instead of later having a separate
assignment statement.

>  static int lapic_save_regs(struct domain *d, hvm_domain_context_t *h)
>  {
>      struct vcpu *v;
> -    struct vlapic *s;
> -    int rc = 0;
> +    int err = 0;
>  
>      if ( !has_vlapic(d) )
>          return 0;
>  
>      for_each_vcpu ( d, v )
>      {
> -        if ( hvm_funcs.sync_pir_to_irr )
> -            hvm_funcs.sync_pir_to_irr(v);
> -
> -        s = vcpu_vlapic(v);
> -        if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) != 0 )
> +        err = lapic_save_regs_one(v, h);
> +        if ( err )
>              break;
>      }
>  
> -    return rc;
> +    return err;
>  }

Since the whole function is meant to go away anyway, it doesn't
matter much, but why did you see a need to replace "rc" by "err"?
This only increases code churn (even if just slightly). IOW: No
need to change this, but something to consider in the future.

Jan



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