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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.05.18 at 13:58, <roger.pau@xxxxxxxxxx> wrote:
> 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.

Feel free.

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

Right, that is the net effect of one of the two TODOs I have in there.

Jan



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