Re: [PATCH v14 06/11] x86/hvm: Introduce hvm_save_mtrr_msr_one func

[Isaila Alexandru <aisaila@bitdefender.com>]

On Ma, 2018-07-31 at 06:16 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 25.07.18 at 14:14, <aisaila@bitdefender.com> wrote:
> > This is used to save data from a single instance.
> > 
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> > 
> > ---
> > Changes since v11:
> > 	- hvm_save_mtrr_msr() now returns err from
> > hvm_save_mtrr_msr_one().
> > 
> > Note: This patch is based on Roger Pau Monne's series[1]
> > ---
> >  xen/arch/x86/hvm/mtrr.c | 81 +++++++++++++++++++++++++++--------
> > --------------
> >  1 file changed, 44 insertions(+), 37 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> > index 48facbb..47a5c29 100644
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -718,52 +718,59 @@ int hvm_set_mem_pinned_cacheattr(struct
> > domain *d, 
> > uint64_t gfn_start,
> >      return 0;
> >  }
> >  
> > -static int hvm_save_mtrr_msr(struct domain *d,
> > hvm_domain_context_t *h)
> > +static int hvm_save_mtrr_msr_one(struct vcpu *v,
> > hvm_domain_context_t *h)
> >  {
> > -    struct vcpu *v;
> > +    const struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
> > +    struct hvm_hw_mtrr hw_mtrr = {
> > +        .msr_mtrr_def_type = mtrr_state->def_type |
> > +                             MASK_INSR(mtrr_state->fixed_enabled,
> > +                                       MTRRdefType_FE) |
> > +                             MASK_INSR(mtrr_state->enabled,
> > MTRRdefType_E),
> > +        .msr_mtrr_cap      = mtrr_state->mtrr_cap,
> > +    };
> > +    unsigned int i;
> >  
> > -    /* save mtrr&pat */
> > -    for_each_vcpu(d, v)
> > +    if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
> > +         (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
> >      {
> > -        const struct mtrr_state *mtrr_state = &v-
> > >arch.hvm_vcpu.mtrr;
> > -        struct hvm_hw_mtrr hw_mtrr = {
> > -            .msr_mtrr_def_type = mtrr_state->def_type |
> > -                                 MASK_INSR(mtrr_state-
> > >fixed_enabled,
> > -                                           MTRRdefType_FE) |
> > -                                 MASK_INSR(mtrr_state->enabled, 
> > MTRRdefType_E),
> > -            .msr_mtrr_cap      = mtrr_state->mtrr_cap,
> > -        };
> > -        unsigned int i;
> > +        dprintk(XENLOG_G_ERR,
> > +                "HVM save: %pv: too many (%lu) variable range
> > MTRRs\n",
> > +                v, MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT));
> > +        return -EINVAL;
> > +    }
> >  
> > -        if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
> > -             (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
> > -        {
> > -            dprintk(XENLOG_G_ERR,
> > -                    "HVM save: %pv: too many (%lu) variable range
> > MTRRs\n",
> > -                    v, MASK_EXTR(hw_mtrr.msr_mtrr_cap,
> > MTRRcap_VCNT));
> > -            return -EINVAL;
> > -        }
> > +    hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
> >  
> > -        hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
> > +    for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap,
> > MTRRcap_VCNT); i++ )
> > +    {
> > +        /* save physbase */
> > +        hw_mtrr.msr_mtrr_var[i*2] =
> > +            ((uint64_t*)mtrr_state->var_ranges)[i*2];
> > +        /* save physmask */
> > +        hw_mtrr.msr_mtrr_var[i*2+1] =
> > +            ((uint64_t*)mtrr_state->var_ranges)[i*2+1];
> > +    }
> As you move/re-indent code, please fix obvious style violations
> (here: missing blanks around binary operators). It also would be
> really nice if you got rid of the ugly and risky casts, and used
> the actual structure fields instead.

Couldn't we just use a 
memcpy(hw_mtrr.msr_mtrr_var, mtrr_state->var_ranges,
MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT)) ?

Same as you suggested...
> 
> > 
> > -        for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap,
> > MTRRcap_VCNT); i++ )
> > -        {
> > -            /* save physbase */
> > -            hw_mtrr.msr_mtrr_var[i*2] =
> > -                ((uint64_t*)mtrr_state->var_ranges)[i*2];
> > -            /* save physmask */
> > -            hw_mtrr.msr_mtrr_var[i*2+1] =
> > -                ((uint64_t*)mtrr_state->var_ranges)[i*2+1];
> > -        }
> > +    for ( i = 0; i < NUM_FIXED_MSR; i++ )
> > +        hw_mtrr.msr_mtrr_fixed[i] =
> > +            ((uint64_t*)mtrr_state->fixed_ranges)[i];
> Whereas here I think you would best simply use memcpy(), again
> to get rid of the cast.
> 
... here

Thanks, 
Alex

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