Re: [Xen-devel] [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

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

On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
> >>> On 10.05.18 at 19:15, <roger.pau@xxxxxxxxxx> wrote:
> > Copy the state found on the hardware when creating a PVH Dom0. Since
> > the memory map provided to a PVH Dom0 is based on the native one using
> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
> > without having to manually build it in Xen.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> > index 95a3deabea..1cb000388a 100644
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -176,6 +176,29 @@ 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]);
> 
> The presence/absence of fixed range MTRRs needs to be reflected in the
> capabilities MSR. Strictly speaking in their absence MSR access attempts to
> the fixed range MSRs should also cause #GP, as should any attempt to
> enable them in defType.

My intention was to always provide the fixed range MTRR capability,
regardless of whether the underlying hardware has it or not. Then of
course fixed ranges won't be enabled by default in the deftype MSR if
the underlying hardware also hasn't got them enabled.

> > +        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 << 10));
> 
> In the interest of no further proliferation of this and similar literal 
> numbers,
> could I ask you to introduce #define-s into msr-index.h?

Sure.

Thanks, Roger.

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