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

[Isaila Alexandru <aisaila@bitdefender.com>]

On Ma, 2018-08-07 at 06:09 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 03.08.18 at 15:53, <aisaila@bitdefender.com> wrote:
> > This is used to save data from a single instance.
> > 
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> > ---
> >  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.
> 
Err was just to have all the functions work with the same variable name
so this was done just for consistency.

Alex

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