Re: [Xen-devel] [PATCH] pvh: change epte_get_entry_emt() for pvh mem types

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 22/11/13 10:52, George Dunlap wrote:
> On 22/11/13 01:00, Mukesh Rathor wrote:
> > For pvh guests, epte_get_entry_emt() is incorrectly returning WB for
> > all mem types because of the following check:
> >      if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> > Skip the check for pvh guests.
> >
> > Also note, MTRR ranges are not maintained for pvh, and a solution is
> > being contrived using PAT.
> >
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> > ---
> >   xen/arch/x86/hvm/mtrr.c |    7 ++++++-
> >   1 files changed, 6 insertions(+), 1 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> > index 4ff1e55..5427e1c 100644
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -693,7 +693,8 @@ uint8_t epte_get_entry_emt(struct domain *d, 
> > unsigned long gfn, mfn_t mfn,
> >            ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) )
> >           return MTRR_TYPE_WRBACK;
> >   -    if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> > +    if ( !is_pvh_vcpu(v) &&
> > + !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> >           return MTRR_TYPE_WRBACK;
> >         if ( !mfn_valid(mfn_x(mfn)) )
> > @@ -717,6 +718,10 @@ uint8_t epte_get_entry_emt(struct domain *d, 
> > unsigned long gfn, mfn_t mfn,
> >           return MTRR_TYPE_WRBACK;
> >       }
> >   +    /* MTRR ranges are not maintained for pvh. */
> > +    if ( is_pvh_vcpu(v) )
> > +        return MTRR_TYPE_WRBACK;
> > +
> >       gmtrr_mtype = get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << 
> > PAGE_SHIFT));
> >       hmtrr_mtype = get_mtrr_type(&mtrr_state, (mfn_x(mfn) << 
> > PAGE_SHIFT));
> >       return ((gmtrr_mtype <= hmtrr_mtype) ? gmtrr_mtype : hmtrr_mtype);
> So this will bypass the host mtrr settings, and always return WRBACK, 
> even if in mtrr_state it was set to something lower. Presumably that 
> "min(host,guest)" was there for a reason.  Are you sure it's safe to 
> just ignore it?
> This is why I suggested the following instead:
>
> gmtrr_mtype = is_hvm_domain(v) ?
>         get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT)) :
>         MTRR_TYPE_WRBACK;
Also, while I suppose this is fine for now (since we're starting the 
code freeze), for the future:  is it actually better to "bake in" that 
MTRRs are completely disabled and always effectively return WRBACK 
(except for direct_mmio memory), or would it be better to initialize the 
virtual MTRRs to WRBACK (the way a BIOS would), and allow the guest to 
change them if it wants?
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel