Re: [Xen-devel] [PATCH] x86/hap: use the right cache attributes when MTRR is disabled

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Mon, Aug 01, 2016 at 09:33:56AM -0600, Jan Beulich wrote:
> >>> On 26.07.16 at 18:15, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -814,10 +814,17 @@ int epte_get_entry_emt(struct domain *d, unsigned 
> > long gfn, mfn_t mfn,
> >      if ( gmtrr_mtype == -EADDRNOTAVAIL )
> >          return -1;
> >  
> > -    gmtrr_mtype = is_hvm_domain(d) && v ?
> 
> Where did the is_hvm_domain() go? Let's not break PVHv1 just yet.

Hm, TBH I don't see why this is needed, AFAICT PVHv1 guests will never have 
MTRR enabled, so they will just go into the MTRR_TYPE_WRBACK case.

> > -                  get_mtrr_type(&v->arch.hvm_vcpu.mtrr,
> > -                                gfn << PAGE_SHIFT, order) :
> > -                  MTRR_TYPE_WRBACK;
> > +    if ( v && v->arch.hvm_vcpu.mtrr.enabled )
> > +        /* MTRR is enabled, use MTRR */
> > +        gmtrr_mtype = get_mtrr_type(&v->arch.hvm_vcpu.mtrr, gfn << 
> > PAGE_SHIFT,
> > +                                    order);
> > +    else if ( v && !hvm_paging_enabled(v) )
> > +        /* MTRR is not enabled and paging is disabled, force UC */
> > +        gmtrr_mtype = MTRR_TYPE_UNCACHABLE;
> > +    else
> > +        /* MTRR is not enabled and paging is enabled, use PAT */
> > +        gmtrr_mtype = MTRR_TYPE_WRBACK;
> 
> I think this would then better be
> 
>     if ( v )

Aren't all guests going to fall into this case, and thus MTRR_TYPE_WRBACK is 
going to be returned without even checking if the guest has MTRR enabled or 
not? Do you mean "if ( !v )" instead?

>         gmtrr_mtype = MTRR_TYPE_WRBACK;
>     else if ( v->arch.hvm_vcpu.mtrr.enabled )
>         /* MTRR is enabled, use MTRR */
>         gmtrr_mtype = get_mtrr_type(&v->arch.hvm_vcpu.mtrr, gfn << PAGE_SHIFT,
>                                     order);
>     else if ( !hvm_paging_enabled(v) )
>         /* MTRR is not enabled and paging is disabled, force UC */
>         gmtrr_mtype = MTRR_TYPE_UNCACHABLE;
>     else
>         /* MTRR is not enabled and paging is enabled, use PAT */
>         gmtrr_mtype = MTRR_TYPE_WRBACK;
> 
> albeit even then using vCPU 0 feels wrong when d != current->domain.
> Plus v->arch.hvm_vcpu.mtrr.enabled isn't really a boolean, so I think
> its use also needs refining.

Yes, I've realized that the check against v->arch.hvm_vcpu.mtrr.enabled is 
wrong, I will change it, but I'm not sure how to fix the d != 
current->domain comments that you mention.

> And finally please fix the comment style.

Right, thanks for the review.

Roger.

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