| Message ID | 1469549737-19668-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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.
>>> 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
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;
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(-)