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

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On Vi, 2018-06-29 at 10:00 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 28.06.18 at 11:25, <aisaila@bitdefender.com> 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?
>

In the first patches(introduce*) it is used only between save*() and
save_one*() funcs but later in patch 9 it is used in hvm_save() so it
has to be visible from both places. I can move the enum(for the return
values) declaration to save.h if that is a good place for it.

Alex

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel