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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.08.18 at 16:57, <aisaila@xxxxxxxxxxxxxxx> wrote:
> On Ma, 2018-07-31 at 06:16 -0600, Jan Beulich wrote:
>> > > > On 25.07.18 at 14:14, <aisaila@xxxxxxxxxxxxxxx> wrote:
>> > --- 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)) ?

Well, strictly speaking we could, but I'd prefer not to here since
the alternative is going to be readable, other than ...

> 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

... here.

Jan


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