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

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

[Roger Pau Monne]

Currently the code that calculates the cache attributes of the HAP page
tables assume that if MTRR are disabled the memory type is UC, this can
cause issues if MTRR are never enabled because the guest only plans to use
PAT.

In order to solve this modify epte_get_entry_emt so that is takes into
account that MTRR can be disabled and that PAT should be used instead, this
also implies that when page tables are enabled inside the guest a
recalculation of the HAP page table cache attributes must be performed.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c  |  4 ++++
 xen/arch/x86/hvm/mtrr.c | 15 +++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index daaee1d..db4b2d6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2283,7 +2283,11 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
         if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
+        {
             paging_update_paging_modes(v);
+            /* Force an update of the memory cache attributes. */
+            memory_type_changed(d);
+        }
     }
 
     return X86EMUL_OKAY;
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index f7831ff..778e85a 100644
--- 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 ?
-                  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;
+
     hmtrr_mtype = get_mtrr_type(&mtrr_state, mfn_x(mfn) << PAGE_SHIFT, order);
     if ( gmtrr_mtype < 0 || hmtrr_mtype < 0 )
         return -1;
-- 
2.7.4 (Apple Git-66)


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

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

[Jan Beulich]

>>> On 26.07.16 at 18:15, <roger.pau@citrix.com> 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.

> -                  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 )
        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.

And finally please fix the comment style.

Jan


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

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

[Roger Pau Monne]

On Mon, Aug 01, 2016 at 09:33:56AM -0600, Jan Beulich wrote:
> >>> On 26.07.16 at 18:15, <roger.pau@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 02.08.16 at 13:25, <roger.pau@citrix.com> wrote:
> On Mon, Aug 01, 2016 at 09:33:56AM -0600, Jan Beulich wrote:
>> >>> On 26.07.16 at 18:15, <roger.pau@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel