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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Ah, good point. But please then briefly mention this in the description.

>> > -                  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?

Indeed I did mean that.

>>         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 I don't think you absolutely need to do something about that
right away, as you're not introducing that issue (you just slightly
widen it).

Jan


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