Re: [Xen-devel] [PATCH v2 4/6] hvm/mtrr: copy hardware state for Dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 16, 2018 at 02:47:39AM -0600, Jan Beulich wrote:
> >>> On 15.05.18 at 16:36, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -185,6 +185,30 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
> >  
> > +    if ( is_hardware_domain(v->domain) )
> > +    {
> > +        /* Copy values from the host. */
> > +        struct domain *d = v->domain;
> > +        unsigned int i;
> > +
> > +        if ( mtrr_state.have_fixed )
> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
> > +                mtrr_fix_range_msr_set(d, m, i,
> > +                                      ((uint64_t 
> > *)mtrr_state.fixed_ranges)[i]);
> > +
> > +        for ( i = 0; i < num_var_ranges; i++ )
> > +        {
> > +            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSBASE(i),
> > +                                   mtrr_state.var_ranges[i].base);
> > +            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSMASK(i),
> > +                                   mtrr_state.var_ranges[i].mask);
> > +        }
> > +
> > +        mtrr_def_type_msr_set(d, m,
> > +                              mtrr_state.def_type |
> > +                              (mtrr_state.enabled << 
> > MTRRdefType_FE_SHIFT));
> 
> This is all very clumsy (not your fault of course) - in particular, the 
> "enabled"
> field is a two-bit value. I'd rather not see this to continue to be that way,
> and hence I've created a patch to clean this up (see below; I'm intentionally
> retaining my own TODO notes in there for your reference, and it's also
> unlikely to apply as is because it sits on top of other changes). In that 
> light I
> think I'd prefer if you either (later) re-based this onto my patch or reverted
> back to the use of the literal 10 here.

I've also realized this but thought about fixing it later.

I don't mind adding this patch to my MTRR series in order to make
rebasing easier on my side if it's OK for you.

I would likely add the enabled and fixed_enabled masks to msr-index.h
and use MASK_* instead of shifts while there.

Thanks, Roger.

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