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

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

On Tue, May 15, 2018 at 02:48:16AM -0600, Jan Beulich wrote:
> >>> On 15.05.18 at 10:35, <roger.pau@xxxxxxxxxx> wrote:
> > On Tue, May 15, 2018 at 01:52:43AM -0600, Jan Beulich wrote:
> >> >>> On 14.05.18 at 18:33, <roger.pau@xxxxxxxxxx> wrote:
> >> > 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.
> >> 
> >> What would the result be of the OS writing to any of these MSRs, or
> >> setting the respective enable bit?
> > 
> > Likely the cache attributes for the guest will change if it sets some
> > fixed ranges and enables the FE bit. But I'm not sure why is that a
> > problem.
> 
> "The guest" being Dom0 here, don't forget. I simply don't see how you
> would properly mimic the behavior without there actually being fixed
> range MTRRs. Plus it contradicts the patch description.

Please bear with me.

The reason of this patchset is to provide PVH Dom0 with a sane initial
MTRR state, not to allow a PVH Dom0 to set the host MTRR state
directly.

So the fact that the underlying hardware doesn't have support for
fixed MTRR ranges shouldn't affect Xen's capability to provide such
feature to Dom0.

I see no reason to allow Dom0 to directly control the host MTRR
values. A PVH Dom0 has it's own physical memory map and can set
whatever cache attributes it wishes without affecting the host MTRR
types.

Thanks, Roger.

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