[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.