[PATCH v4 0/8] PVH MTRR initial state

[PATCH v4 0/8] PVH MTRR initial state

[Roger Pau Monne]

Hello,

The following patches set a sane initial MTRR state for both Dom0 and
DomU PVH guests. Note that for Dom0 the host MTRR state is used, OTOH
for DomU the default MTRR type is set to write-back.

This should avoid guests having to setup some kind of MTRR state in
order to boot.

This has been rebased on top of a couple of fixes/improvements from Jan,
which are also included in the series.

Thanks, Roger.

Jan Beulich (2):
  x86/HVM: improve MTRR load checks
  x86/mtrr: split "enabled" field into two boolean flags

Roger Pau Monne (6):
  mtrr: introduce mask to get VCNT from MTRRcap MSR
  hvm/mtrr: add emacs local variables block with formatting info
  hvm/mtrr: use the hardware number of variable ranges for Dom0
  hvm/mtrr: copy hardware state for Dom0
  libxc/pvh: set default MTRR type to write-back
  docs/pvh: document initial MTRR state

 docs/misc/pvh.markdown          |  18 +++++
 tools/libxc/xc_dom_x86.c        |  44 ++++++++++++
 xen/arch/x86/cpu/mtrr/generic.c |  14 ++--
 xen/arch/x86/cpu/mtrr/main.c    |   2 +-
 xen/arch/x86/hvm/hvm.c          |  18 +++--
 xen/arch/x86/hvm/mtrr.c         | 124 +++++++++++++++++++++++++-------
 xen/include/asm-x86/msr-index.h |   4 ++
 xen/include/asm-x86/mtrr.h      |   8 ++-
 8 files changed, 195 insertions(+), 37 deletions(-)

-- 
2.17.1


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

[PATCH v4 1/8] mtrr: introduce mask to get VCNT from MTRRcap MSR

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v3:
 - Rebase on top of Jan's MTRR fixes.

Changes since v2:
 - Use unsigned int instead of uint8_t in mtrr_pat_not_equal.
---
 xen/arch/x86/cpu/mtrr/main.c    | 2 +-
 xen/arch/x86/hvm/mtrr.c         | 8 ++++----
 xen/include/asm-x86/msr-index.h | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/main.c b/xen/arch/x86/cpu/mtrr/main.c
index 56f71a6e1f..e9df53f00d 100644
--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -95,7 +95,7 @@ static void __init set_num_var_ranges(void)
 		config = 2;
 	else if (is_cpu(CENTAUR))
 		config = 8;
-	num_var_ranges = config & 0xff;
+	num_var_ranges = MASK_EXTR(config, MTRRcap_VCNT);
 }
 
 static void __init init_table(void)
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index a61cc1e6dc..c298369044 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -78,7 +78,7 @@ static uint8_t __read_mostly pat_entry_tbl[PAT_TYPE_NUMS] =
 bool_t is_var_mtrr_overlapped(const struct mtrr_state *m)
 {
     unsigned int seg, i;
-    unsigned int num_var_ranges = (uint8_t)m->mtrr_cap;
+    unsigned int num_var_ranges = MASK_EXTR(m->mtrr_cap, MTRRcap_VCNT);
 
     for ( i = 0; i < num_var_ranges; i++ )
     {
@@ -193,7 +193,7 @@ static int get_mtrr_type(const struct mtrr_state *m,
    uint8_t     overlap_mtrr = 0;
    uint8_t     overlap_mtrr_pos = 0;
    uint64_t    mask = -(uint64_t)PAGE_SIZE << order;
-   unsigned int seg, num_var_ranges = m->mtrr_cap & 0xff;
+   unsigned int seg, num_var_ranges = MASK_EXTR(m->mtrr_cap, MTRRcap_VCNT);
 
    if ( unlikely(!(m->enabled & 0x2)) )
        return MTRR_TYPE_UNCACHABLE;
@@ -483,7 +483,7 @@ bool mtrr_pat_not_equal(const struct vcpu *vd, const struct vcpu *vs)
 
     if ( md->enabled & 2 )
     {
-        unsigned int num_var_ranges = (uint8_t)md->mtrr_cap;
+        unsigned int num_var_ranges = MASK_EXTR(md->mtrr_cap, MTRRcap_VCNT);
 
         /* Test default type MSR. */
         if ( md->def_type != ms->def_type )
@@ -499,7 +499,7 @@ bool mtrr_pat_not_equal(const struct vcpu *vd, const struct vcpu *vs)
             return true;
 
         /* Test variable ranges. */
-        if ( num_var_ranges != (uint8_t)ms->mtrr_cap ||
+        if ( num_var_ranges != MASK_EXTR(ms->mtrr_cap, MTRRcap_VCNT) ||
              memcmp(md->var_ranges, ms->var_ranges,
                     num_var_ranges * sizeof(*md->var_ranges)) )
             return true;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 8fbccc88a7..95bb66916c 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -60,6 +60,8 @@
 #define ATM_LNC_C6_AUTO_DEMOTE		(1UL << 25)
 
 #define MSR_MTRRcap			0x000000fe
+#define MTRRcap_VCNT			0x000000ff
+
 #define MSR_IA32_BBL_CR_CTL		0x00000119
 
 #define MSR_IA32_SYSENTER_CS		0x00000174
-- 
2.17.1


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

[PATCH v4 2/8] x86/HVM: improve MTRR load checks

[Roger Pau Monne]

From: Jan Beulich <jbeulich@suse.com>

We should not assume that the incoming set of values contains exactly
MTRR_VCNT variable range MSRs. Permit a smaller amount and reject a
bigger one. As a result the save path then also needs to no longer use
a fixed upper bound, in turn requiring unused space in the save record
to be zeroed up front.

Also slightly refine types where appropriate.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
[switch to use MASK_EXTR to get VCNT]
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/mtrr.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index c298369044..654299a198 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -673,22 +673,22 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
 
 static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 {
-    int i;
     struct vcpu *v;
-    struct hvm_hw_mtrr hw_mtrr;
-    struct mtrr_state *mtrr_state;
+
     /* save mtrr&pat */
     for_each_vcpu(d, v)
     {
-        mtrr_state = &v->arch.hvm_vcpu.mtrr;
+        const struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
+        struct hvm_hw_mtrr hw_mtrr = {
+            .msr_mtrr_def_type = mtrr_state->def_type |
+                                 (mtrr_state->enabled << 10),
+            .msr_mtrr_cap      = mtrr_state->mtrr_cap,
+        };
+        unsigned int i;
 
         hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
 
-        hw_mtrr.msr_mtrr_def_type = mtrr_state->def_type
-                                | (mtrr_state->enabled << 10);
-        hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
-
-        for ( i = 0; i < MTRR_VCNT; i++ )
+        for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
         {
             /* save physbase */
             hw_mtrr.msr_mtrr_var[i*2] =
@@ -726,6 +726,14 @@ static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
     if ( hvm_load_entry(MTRR, h, &hw_mtrr) != 0 )
         return -EINVAL;
 
+    if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) > MTRR_VCNT )
+    {
+        dprintk(XENLOG_G_ERR,
+                "HVM restore: %pv: too many (%lu) variable range MTRRs\n",
+                v, MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT));
+        return -EINVAL;
+    }
+
     mtrr_state = &v->arch.hvm_vcpu.mtrr;
 
     hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
@@ -735,7 +743,7 @@ static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
     for ( i = 0; i < NUM_FIXED_MSR; i++ )
         mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
 
-    for ( i = 0; i < MTRR_VCNT; i++ )
+    for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
     {
         mtrr_var_range_msr_set(d, mtrr_state,
                                MSR_IA32_MTRR_PHYSBASE(i),
-- 
2.17.1


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

[PATCH v4 3/8] x86/mtrr: split "enabled" field into two boolean flags

[Roger Pau Monne]

From: Jan Beulich <jbeulich@suse.com>

The code hopefully is more readable this way.

Also switch have_fixed to bool, seeing that it already is used as a
boolean.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
[switched to use MASK_*]
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/cpu/mtrr/generic.c | 14 +++++++++-----
 xen/arch/x86/hvm/hvm.c          |  6 ++++--
 xen/arch/x86/hvm/mtrr.c         | 23 ++++++++++++++---------
 xen/include/asm-x86/msr-index.h |  2 ++
 xen/include/asm-x86/mtrr.h      |  5 +++--
 5 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 7ba0c3f0fe..09763654be 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -80,7 +80,8 @@ void __init get_mtrr_state(void)
 
 	rdmsrl(MSR_MTRRdefType, msr_content);
 	mtrr_state.def_type = (msr_content & 0xff);
-	mtrr_state.enabled = (msr_content & 0xc00) >> 10;
+	mtrr_state.enabled = MASK_EXTR(msr_content, MTRRdefType_E);
+	mtrr_state.fixed_enabled = MASK_EXTR(msr_content, MTRRdefType_FE);
 
 	/* Store mtrr_cap for HVM MTRR virtualisation. */
 	rdmsrl(MSR_MTRRcap, mtrr_state.mtrr_cap);
@@ -159,7 +160,7 @@ static void __init print_mtrr_state(const char *level)
 		unsigned int base = 0, step = 0x10000;
 
 		printk("%sMTRR fixed ranges %sabled:\n", level,
-		       mtrr_state.enabled & 1 ? "en" : "dis");
+		       mtrr_state.fixed_enabled ? "en" : "dis");
 		for (; block->ranges; ++block, step >>= 2) {
 			for (i = 0; i < block->ranges; ++i, fr += 8) {
 				print_fixed(base, step, fr, level);
@@ -169,7 +170,7 @@ static void __init print_mtrr_state(const char *level)
 		print_fixed_last(level);
 	}
 	printk("%sMTRR variable ranges %sabled:\n", level,
-	       mtrr_state.enabled & 2 ? "en" : "dis");
+	       mtrr_state.enabled ? "en" : "dis");
 	width = (paddr_bits - PAGE_SHIFT + 3) / 4;
 
 	for (i = 0; i < num_var_ranges; ++i) {
@@ -383,8 +384,11 @@ static unsigned long set_mtrr_state(void)
 	/*  Set_mtrr_restore restores the old value of MTRRdefType,
 	   so to set it we fiddle with the saved value  */
 	if ((deftype & 0xff) != mtrr_state.def_type
-	    || ((deftype & 0xc00) >> 10) != mtrr_state.enabled) {
-		deftype = (deftype & ~0xcff) | mtrr_state.def_type | (mtrr_state.enabled << 10);
+	    || MASK_EXTR(deftype, MTRRdefType_E) != mtrr_state.enabled
+	    || MASK_EXTR(deftype, MTRRdefType_FE) != mtrr_state.fixed_enabled) {
+		deftype = (deftype & ~0xcff) | mtrr_state.def_type |
+		          MASK_INSR(mtrr_state.enabled, MTRRdefType_E) |
+		          MASK_INSR(mtrr_state.fixed_enabled, MTRRdefType_FE);
 		change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
 	}
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c23983cdff..247b3eb1bd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3468,8 +3468,10 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     case MSR_MTRRdefType:
         if ( !d->arch.cpuid->basic.mtrr )
             goto gp_fault;
-        *msr_content = v->arch.hvm_vcpu.mtrr.def_type
-                        | (v->arch.hvm_vcpu.mtrr.enabled << 10);
+        *msr_content = v->arch.hvm_vcpu.mtrr.def_type |
+                       MASK_INSR(v->arch.hvm_vcpu.mtrr.enabled, MTRRdefType_E) |
+                       MASK_INSR(v->arch.hvm_vcpu.mtrr.fixed_enabled,
+                                 MTRRdefType_FE);
         break;
     case MSR_MTRRfix64K_00000:
         if ( !d->arch.cpuid->basic.mtrr )
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 654299a198..c181a7a3d0 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -195,11 +195,11 @@ static int get_mtrr_type(const struct mtrr_state *m,
    uint64_t    mask = -(uint64_t)PAGE_SIZE << order;
    unsigned int seg, num_var_ranges = MASK_EXTR(m->mtrr_cap, MTRRcap_VCNT);
 
-   if ( unlikely(!(m->enabled & 0x2)) )
+   if ( unlikely(!m->enabled) )
        return MTRR_TYPE_UNCACHABLE;
 
    pa &= mask;
-   if ( (pa < 0x100000) && (m->enabled & 1) )
+   if ( (pa < 0x100000) && m->fixed_enabled )
    {
        /* Fixed range MTRR takes effect. */
        uint32_t addr = (uint32_t)pa, index;
@@ -391,7 +391,8 @@ bool_t mtrr_def_type_msr_set(struct domain *d, struct mtrr_state *m,
                              uint64_t msr_content)
 {
     uint8_t def_type = msr_content & 0xff;
-    uint8_t enabled = (msr_content >> 10) & 0x3;
+    bool fixed_enabled = MASK_EXTR(msr_content, MTRRdefType_FE);
+    bool enabled = MASK_EXTR(msr_content, MTRRdefType_E);
 
     if ( unlikely(!valid_mtrr_type(def_type)) )
     {
@@ -406,10 +407,12 @@ bool_t mtrr_def_type_msr_set(struct domain *d, struct mtrr_state *m,
          return 0;
     }
 
-    if ( m->enabled != enabled || m->def_type != def_type )
+    if ( m->enabled != enabled || m->fixed_enabled != fixed_enabled ||
+         m->def_type != def_type )
     {
         m->enabled = enabled;
         m->def_type = def_type;
+        m->fixed_enabled = fixed_enabled;
         memory_type_changed(d);
     }
 
@@ -478,10 +481,10 @@ bool mtrr_pat_not_equal(const struct vcpu *vd, const struct vcpu *vs)
     const struct mtrr_state *md = &vd->arch.hvm_vcpu.mtrr;
     const struct mtrr_state *ms = &vs->arch.hvm_vcpu.mtrr;
 
-    if ( (md->enabled ^ ms->enabled) & 2 )
+    if ( md->enabled != ms->enabled )
         return true;
 
-    if ( md->enabled & 2 )
+    if ( md->enabled )
     {
         unsigned int num_var_ranges = MASK_EXTR(md->mtrr_cap, MTRRcap_VCNT);
 
@@ -490,10 +493,10 @@ bool mtrr_pat_not_equal(const struct vcpu *vd, const struct vcpu *vs)
             return true;
 
         /* Test fixed ranges. */
-        if ( (md->enabled ^ ms->enabled) & 1 )
+        if ( md->fixed_enabled != ms->fixed_enabled )
             return true;
 
-        if ( (md->enabled & 1) &&
+        if ( md->fixed_enabled &&
              memcmp(md->fixed_ranges, ms->fixed_ranges,
                     sizeof(md->fixed_ranges)) )
             return true;
@@ -681,7 +684,9 @@ static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
         const struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
         struct hvm_hw_mtrr hw_mtrr = {
             .msr_mtrr_def_type = mtrr_state->def_type |
-                                 (mtrr_state->enabled << 10),
+                                 MASK_INSR(mtrr_state->fixed_enabled,
+                                           MTRRdefType_FE) |
+                                 MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
             .msr_mtrr_cap      = mtrr_state->mtrr_cap,
         };
         unsigned int i;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 95bb66916c..94bccf73a1 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -98,6 +98,8 @@
 #define MSR_MTRRfix4K_F0000		0x0000026e
 #define MSR_MTRRfix4K_F8000		0x0000026f
 #define MSR_MTRRdefType			0x000002ff
+#define MTRRdefType_FE			(1u << 10)
+#define MTRRdefType_E			(1u << 11)
 
 #define MSR_IA32_DEBUGCTLMSR		0x000001d9
 #define IA32_DEBUGCTLMSR_LBR		(1<<0) /* Last Branch Record */
diff --git a/xen/include/asm-x86/mtrr.h b/xen/include/asm-x86/mtrr.h
index 5cdc5d4fe3..b1f7af6396 100644
--- a/xen/include/asm-x86/mtrr.h
+++ b/xen/include/asm-x86/mtrr.h
@@ -50,8 +50,9 @@ struct mtrr_var_range {
 struct mtrr_state {
 	struct mtrr_var_range *var_ranges;
 	mtrr_type fixed_ranges[NUM_FIXED_RANGES];
-	unsigned char enabled;
-	unsigned char have_fixed;
+	bool enabled;
+	bool fixed_enabled;
+	bool have_fixed;
 	mtrr_type def_type;
 
 	u64       mtrr_cap;
-- 
2.17.1


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

[PATCH v4 4/8] hvm/mtrr: add emacs local variables block with formatting info

[Roger Pau Monne]

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/mtrr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index c181a7a3d0..2f8f8ddd8f 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -871,3 +871,13 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     return MTRR_TYPE_UNCACHABLE;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1


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

[PATCH v4 5/8] hvm/mtrr: use the hardware number of variable ranges for Dom0

[Roger Pau Monne]

Expand the size of the variable ranges array to match the size of the
underlying hardware, this is a preparatory change for copying the
hardware MTRR state for Dom0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v3:
 - Move the index check to hvm_msr_write_intercept.

Changes since v2:
 - Use Dom%u in hvm_vcpu_cacheattr_init error message.
 - Print debug error messages in hvm_save_mtrr_msr.
 - Use ARRAY_SIZE in hvm_save_mtrr_msr to calculate the number of MTRR
   ranges in the save record.
 - Calculate MTRR_VCNT_MAX based on current MSR indexes.

Changes since v1:
 - Fix hvm_msr_{read,write}_intercept().
 - Relax the checks in hvm_{save/load}_mtrr_msr.
---
 xen/arch/x86/hvm/hvm.c     | 12 +++++++++---
 xen/arch/x86/hvm/mtrr.c    | 31 +++++++++++++++++++++++++++++--
 xen/include/asm-x86/mtrr.h |  3 +++
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 247b3eb1bd..93092d2bb8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3491,10 +3491,13 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         index = msr - MSR_MTRRfix4K_C0000;
         *msr_content = fixed_range_base[index + 3];
         break;
-    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT-1):
+    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT_MAX - 1):
         if ( !d->arch.cpuid->basic.mtrr )
             goto gp_fault;
         index = msr - MSR_IA32_MTRR_PHYSBASE(0);
+        if ( (index / 2) >=
+             MASK_EXTR(v->arch.hvm_vcpu.mtrr.mtrr_cap, MTRRcap_VCNT) )
+            goto gp_fault;
         *msr_content = var_range_base[index];
         break;
 
@@ -3652,10 +3655,13 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
                                      index, msr_content) )
             goto gp_fault;
         break;
-    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT-1):
+    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT_MAX - 1):
         if ( !d->arch.cpuid->basic.mtrr )
             goto gp_fault;
-        if ( !mtrr_var_range_msr_set(v->domain, &v->arch.hvm_vcpu.mtrr,
+        index = msr - MSR_IA32_MTRR_PHYSBASE(0);
+        if ( ((index / 2) >= MASK_EXTR(v->arch.hvm_vcpu.mtrr.mtrr_cap,
+                                       MTRRcap_VCNT)) ||
+             !mtrr_var_range_msr_set(v->domain, &v->arch.hvm_vcpu.mtrr,
                                      msr, msr_content) )
             goto gp_fault;
         break;
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 2f8f8ddd8f..54dede0ea6 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -154,14 +154,26 @@ uint8_t pat_type_2_pte_flags(uint8_t pat_type)
 int hvm_vcpu_cacheattr_init(struct vcpu *v)
 {
     struct mtrr_state *m = &v->arch.hvm_vcpu.mtrr;
+    unsigned int num_var_ranges =
+        is_hardware_domain(v->domain) ? MASK_EXTR(mtrr_state.mtrr_cap,
+                                                  MTRRcap_VCNT)
+                                      : MTRR_VCNT;
+
+    if ( num_var_ranges > MTRR_VCNT_MAX )
+    {
+        ASSERT(is_hardware_domain(v->domain));
+        printk("WARNING: limited Dom%u variable range MTRRs from %u to %u\n",
+               v->domain->domain_id, num_var_ranges, MTRR_VCNT_MAX);
+        num_var_ranges = MTRR_VCNT_MAX;
+    }
 
     memset(m, 0, sizeof(*m));
 
-    m->var_ranges = xzalloc_array(struct mtrr_var_range, MTRR_VCNT);
+    m->var_ranges = xzalloc_array(struct mtrr_var_range, num_var_ranges);
     if ( m->var_ranges == NULL )
         return -ENOMEM;
 
-    m->mtrr_cap = (1u << 10) | (1u << 8) | MTRR_VCNT;
+    m->mtrr_cap = (1u << 10) | (1u << 8) | num_var_ranges;
 
     v->arch.hvm_vcpu.pat_cr =
         ((uint64_t)PAT_TYPE_WRBACK) |               /* PAT0: WB */
@@ -448,6 +460,12 @@ bool_t mtrr_var_range_msr_set(
     uint64_t *var_range_base = (uint64_t*)m->var_ranges;
 
     index = msr - MSR_IA32_MTRR_PHYSBASE(0);
+    if ( (index / 2) >= MASK_EXTR(m->mtrr_cap, MTRRcap_VCNT) )
+    {
+        ASSERT_UNREACHABLE();
+        return 0;
+    }
+
     if ( var_range_base[index] == msr_content )
         return 1;
 
@@ -691,6 +709,15 @@ static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
         };
         unsigned int i;
 
+        if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
+             (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
+        {
+            dprintk(XENLOG_G_ERR,
+                    "HVM save: %pv: too many (%lu) variable range MTRRs\n",
+                    v, MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT));
+            return -EINVAL;
+        }
+
         hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
 
         for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
diff --git a/xen/include/asm-x86/mtrr.h b/xen/include/asm-x86/mtrr.h
index b1f7af6396..72d0690e28 100644
--- a/xen/include/asm-x86/mtrr.h
+++ b/xen/include/asm-x86/mtrr.h
@@ -39,6 +39,9 @@ typedef u8 mtrr_type;
 #define MTRR_PHYSBASE_SHIFT      12
 /* Number of variable range MSR pairs we emulate for HVM guests: */
 #define MTRR_VCNT                8
+/* Maximum number of variable range MSR pairs if FE is supported. */
+#define MTRR_VCNT_MAX            ((MSR_MTRRfix64K_00000 - \
+                                   MSR_IA32_MTRR_PHYSBASE(0)) / 2)
 
 struct mtrr_var_range {
 	uint64_t base;
-- 
2.17.1


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

[PATCH v4 6/8] hvm/mtrr: copy hardware state for Dom0

[Roger Pau Monne]

Copy the state found on the hardware when creating a PVH Dom0. Since
the memory map provided to a PVH Dom0 is based on the native one using
the same set of MTRR ranges should provide Dom0 with a sane MTRR state
without having to manually build it in Xen.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
 - Introduce and use the FE shift into the deftype MTRR MSR.
---
 xen/arch/x86/hvm/mtrr.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 54dede0ea6..48facbb32a 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -185,6 +185,32 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
         ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
         ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
 
+    if ( is_hardware_domain(v->domain) )
+    {
+        /* Copy values from the host. */
+        struct domain *d = v->domain;
+        unsigned int i;
+
+        if ( mtrr_state.have_fixed )
+            for ( i = 0; i < NUM_FIXED_MSR; i++ )
+                mtrr_fix_range_msr_set(d, m, i,
+                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
+
+        for ( i = 0; i < num_var_ranges; i++ )
+        {
+            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSBASE(i),
+                                   mtrr_state.var_ranges[i].base);
+            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSMASK(i),
+                                   mtrr_state.var_ranges[i].mask);
+        }
+
+        mtrr_def_type_msr_set(d, m,
+                              mtrr_state.def_type |
+                              MASK_INSR(mtrr_state.fixed_enabled,
+                                        MTRRdefType_FE) |
+                              MASK_INSR(mtrr_state.enabled, MTRRdefType_E));
+    }
+
     return 0;
 }
 
-- 
2.17.1


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

[PATCH v4 7/8] libxc/pvh: set default MTRR type to write-back

[Roger Pau Monne]

And enable MTRR. This allows to provide a sane initial MTRR state for
PVH DomUs. This will have to be expanded when pci-passthrough support
is added to PVH guests, so that MMIO regions of devices are set as
UC.

Note that initial MTRR setup is done by hvmloader for HVM guests,
that's not used by PVH guests.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
----
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e33a28847d..d28ff4d7e9 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -53,6 +53,9 @@
 #define X86_CR0_PE 0x01
 #define X86_CR0_ET 0x10
 
+#define MTRR_TYPE_WRBACK     6
+#define MTRR_DEF_TYPE_ENABLE (1u << 11)
+
 #define SPECIALPAGE_PAGING   0
 #define SPECIALPAGE_ACCESS   1
 #define SPECIALPAGE_SHARING  2
@@ -931,6 +934,20 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
     return rc;
 }
 
+const static void *hvm_get_save_record(const void *ctx, unsigned int type,
+                                       unsigned int instance)
+{
+    const struct hvm_save_descriptor *header;
+
+    for ( header = ctx;
+          header->typecode != HVM_SAVE_CODE(END);
+          ctx += sizeof(*header) + header->length, header = ctx )
+        if ( header->typecode == type && header->instance == instance )
+            return ctx + sizeof(*header);
+
+    return NULL;
+}
+
 static int vcpu_hvm(struct xc_dom_image *dom)
 {
     struct {
@@ -938,9 +955,12 @@ static int vcpu_hvm(struct xc_dom_image *dom)
         HVM_SAVE_TYPE(HEADER) header;
         struct hvm_save_descriptor cpu_d;
         HVM_SAVE_TYPE(CPU) cpu;
+        struct hvm_save_descriptor mtrr_d;
+        HVM_SAVE_TYPE(MTRR) mtrr;
         struct hvm_save_descriptor end_d;
         HVM_SAVE_TYPE(END) end;
     } bsp_ctx;
+    const HVM_SAVE_TYPE(MTRR) *mtrr_record;
     uint8_t *full_ctx = NULL;
     int rc;
 
@@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
     if ( dom->start_info_seg.pfn )
         bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
 
+    /* Set the MTRR. */
+    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
+    bsp_ctx.mtrr_d.instance = 0;
+    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
+
+    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
+    if ( !mtrr_record )
+    {
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "%s: unable to get MTRR save record", __func__);
+        goto out;
+    }
+
+    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
+
+    /* TODO: maybe this should be a firmware option instead? */
+    if ( !dom->device_model )
+        /*
+         * Enable MTRR, set default type to WB.
+         * TODO: add MMIO areas as UC when passthrough is supported.
+         */
+        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
+                                         MTRR_DEF_TYPE_ENABLE;
+
     /* Set the end descriptor. */
     bsp_ctx.end_d.typecode = HVM_SAVE_CODE(END);
     bsp_ctx.end_d.instance = 0;
-- 
2.17.1


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

[PATCH v4 8/8] docs/pvh: document initial MTRR state

[Roger Pau Monne]

Provided to both Dom0 and DomUs.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v2:
 - Add 'currently' to the first sentence about the default MTRR type.

Changes since v1:
 - Add an extra paragraph to clarify the initial MTRR state.
---
 docs/misc/pvh.markdown | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/docs/misc/pvh.markdown b/docs/misc/pvh.markdown
index e85fb15374..1c9a00b48a 100644
--- a/docs/misc/pvh.markdown
+++ b/docs/misc/pvh.markdown
@@ -92,3 +92,21 @@ event channels. Delivery of those interrupts can be configured in the same way
 as HVM guests, check xen/include/public/hvm/params.h and
 xen/include/public/hvm/hvm\_op.h for more information about available delivery
 methods.
+
+## MTRR ##
+
+### Unprivileged guests ###
+
+PVH guests are currently booted with the default MTRR type set to write-back
+and MTRR enabled. This allows DomUs to start with a sane MTRR state. Note that
+this will have to be revisited when pci-passthrough is added to PVH in order to
+set MMIO regions as UC.
+
+Xen guarantees that RAM regions will always have the WB cache type set in the
+initial MTRR state, either set by the default MTRR type or by other means.
+
+### Hardware domain ###
+
+A PVH hardware domain is booted with the same MTRR state as the one found on
+the host. This is done because the hardware domain memory map is already a
+modified copy of the host memory map, so the same MTRR setup should work.
-- 
2.17.1


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

Re: [PATCH v4 5/8] hvm/mtrr: use the hardware number of variable ranges for Dom0

[Jan Beulich]

>>> On 08.06.18 at 11:59, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3491,10 +3491,13 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>          index = msr - MSR_MTRRfix4K_C0000;
>          *msr_content = fixed_range_base[index + 3];
>          break;
> -    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT-1):
> +    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT_MAX - 1):
>          if ( !d->arch.cpuid->basic.mtrr )
>              goto gp_fault;
>          index = msr - MSR_IA32_MTRR_PHYSBASE(0);
> +        if ( (index / 2) >=
> +             MASK_EXTR(v->arch.hvm_vcpu.mtrr.mtrr_cap, MTRRcap_VCNT) )

I'd prefer if the line wrapping here and ...

> @@ -3652,10 +3655,13 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
>                                       index, msr_content) )
>              goto gp_fault;
>          break;
> -    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT-1):
> +    case MSR_IA32_MTRR_PHYSBASE(0)...MSR_IA32_MTRR_PHYSMASK(MTRR_VCNT_MAX - 1):
>          if ( !d->arch.cpuid->basic.mtrr )
>              goto gp_fault;
> -        if ( !mtrr_var_range_msr_set(v->domain, &v->arch.hvm_vcpu.mtrr,
> +        index = msr - MSR_IA32_MTRR_PHYSBASE(0);
> +        if ( ((index / 2) >= MASK_EXTR(v->arch.hvm_vcpu.mtrr.mtrr_cap,
> +                                       MTRRcap_VCNT)) ||

... here was done as similarly as possible, to make it as easy as possible for
the reader to recognize this is twice the same expression.

With that (and this is easy enough to be done while committing, so no reason
on its own to send another version)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH v4 0/8] PVH MTRR initial state

[Roger Pau Monné]

On Fri, Jun 08, 2018 at 11:59:26AM +0200, Roger Pau Monne wrote:
> Hello,
> 
> The following patches set a sane initial MTRR state for both Dom0 and
> DomU PVH guests. Note that for Dom0 the host MTRR state is used, OTOH
> for DomU the default MTRR type is set to write-back.
> 
> This should avoid guests having to setup some kind of MTRR state in
> order to boot.
> 
> This has been rebased on top of a couple of fixes/improvements from Jan,
> which are also included in the series.
> 
> Thanks, Roger.
> 
> Jan Beulich (2):
>   x86/HVM: improve MTRR load checks
>   x86/mtrr: split "enabled" field into two boolean flags
> 
> Roger Pau Monne (6):
>   mtrr: introduce mask to get VCNT from MTRRcap MSR
>   hvm/mtrr: add emacs local variables block with formatting info
>   hvm/mtrr: use the hardware number of variable ranges for Dom0
>   hvm/mtrr: copy hardware state for Dom0
>   libxc/pvh: set default MTRR type to write-back
>   docs/pvh: document initial MTRR state

Hello,

Patches #2, #3, #4 and #8 don't have any reviews yet. Now that the
vpci MSI fixes have been committed the initial MTRR state is the last
chunk missing in order to get initial PVH Dom0 support fully
functional.

Thanks, Roger.

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

Re: [PATCH v4 0/8] PVH MTRR initial state

[Jan Beulich]

>>> On 03.07.18 at 10:17, <roger.pau@citrix.com> wrote:
> On Fri, Jun 08, 2018 at 11:59:26AM +0200, Roger Pau Monne wrote:
>> Hello,
>> 
>> The following patches set a sane initial MTRR state for both Dom0 and
>> DomU PVH guests. Note that for Dom0 the host MTRR state is used, OTOH
>> for DomU the default MTRR type is set to write-back.
>> 
>> This should avoid guests having to setup some kind of MTRR state in
>> order to boot.
>> 
>> This has been rebased on top of a couple of fixes/improvements from Jan,
>> which are also included in the series.
>> 
>> Thanks, Roger.
>> 
>> Jan Beulich (2):
>>   x86/HVM: improve MTRR load checks
>>   x86/mtrr: split "enabled" field into two boolean flags
>> 
>> Roger Pau Monne (6):
>>   mtrr: introduce mask to get VCNT from MTRRcap MSR
>>   hvm/mtrr: add emacs local variables block with formatting info
>>   hvm/mtrr: use the hardware number of variable ranges for Dom0
>>   hvm/mtrr: copy hardware state for Dom0
>>   libxc/pvh: set default MTRR type to write-back
>>   docs/pvh: document initial MTRR state
> 
> Hello,
> 
> Patches #2, #3, #4 and #8 don't have any reviews yet.

I don't think #4 needs any review. If I was to commit the series (i.e. if
Andrew gave an ack on #2 and #3) I'd put this in without any further
tags. I've been considering the same for #8.

But committing will have to wait anyway until after 4.11 was tagged.

Jan



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

Re: [PATCH v4 7/8] libxc/pvh: set default MTRR type to write-back

[Andrew Cooper]

On 08/06/18 10:59, Roger Pau Monne wrote:
> @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>      if ( dom->start_info_seg.pfn )
>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>  
> +    /* Set the MTRR. */
> +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> +    bsp_ctx.mtrr_d.instance = 0;
> +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> +
> +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> +    if ( !mtrr_record )
> +    {
> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                     "%s: unable to get MTRR save record", __func__);
> +        goto out;
> +    }
> +
> +    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
> +
> +    /* TODO: maybe this should be a firmware option instead? */
> +    if ( !dom->device_model )
> +        /*
> +         * Enable MTRR, set default type to WB.
> +         * TODO: add MMIO areas as UC when passthrough is supported.
> +         */
> +        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
> +                                         MTRR_DEF_TYPE_ENABLE;

This is buggy.  MTRRs are per-vcpu and expected to match.  This only
works by chance in the HVM case because all settings are still 0 at this
point.

Currently, booting a multi-vcpu PVH guest (the shim, specifically) yields:

(d6) (XEN) mtrr: your CPUs had inconsistent MTRRdefType settings
(d6) (XEN) mtrr: probably your BIOS does not setup all CPUs.
(d6) (XEN) mtrr: corrected configuration.
(d6) (XEN) MTRR default type: write-back
(d6) (XEN) MTRR fixed ranges disabled:
(d6) (XEN)   00000-fffff uncachable
(d6) (XEN) MTRR variable ranges enabled:
(d6) (XEN)   0 disabled
(d6) (XEN)   1 disabled
(d6) (XEN)   2 disabled
(d6) (XEN)   3 disabled
(d6) (XEN)   4 disabled
(d6) (XEN)   5 disabled
(d6) (XEN)   6 disabled
(d6) (XEN)   7 disabled

~Andrew

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

Re: [PATCH v4 7/8] libxc/pvh: set default MTRR type to write-back

[Roger Pau Monné]

On Wed, Sep 26, 2018 at 01:29:27PM +0100, Andrew Cooper wrote:
> On 08/06/18 10:59, Roger Pau Monne wrote:
> > @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
> >      if ( dom->start_info_seg.pfn )
> >          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
> >  
> > +    /* Set the MTRR. */
> > +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> > +    bsp_ctx.mtrr_d.instance = 0;
> > +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> > +
> > +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> > +    if ( !mtrr_record )
> > +    {
> > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> > +                     "%s: unable to get MTRR save record", __func__);
> > +        goto out;
> > +    }
> > +
> > +    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
> > +
> > +    /* TODO: maybe this should be a firmware option instead? */
> > +    if ( !dom->device_model )
> > +        /*
> > +         * Enable MTRR, set default type to WB.
> > +         * TODO: add MMIO areas as UC when passthrough is supported.
> > +         */
> > +        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
> > +                                         MTRR_DEF_TYPE_ENABLE;
> 
> This is buggy.  MTRRs are per-vcpu and expected to match.  This only
> works by chance in the HVM case because all settings are still 0 at this
> point.

Yes, I will look into it ASAP, but I'm not sure if the current
save/restore trick that's done here will be possible with vcpus that
are down or I will need to expand the save/restore logic to allow
setting the state of vcpus that are down.

Thanks, Roger.

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

Re: [PATCH v4 7/8] libxc/pvh: set default MTRR type to write-back

[Andrew Cooper]

On 01/10/18 16:52, Roger Pau Monné wrote:
> On Wed, Sep 26, 2018 at 01:29:27PM +0100, Andrew Cooper wrote:
>> On 08/06/18 10:59, Roger Pau Monne wrote:
>>> @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>>      if ( dom->start_info_seg.pfn )
>>>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>>>  
>>> +    /* Set the MTRR. */
>>> +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
>>> +    bsp_ctx.mtrr_d.instance = 0;
>>> +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
>>> +
>>> +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
>>> +    if ( !mtrr_record )
>>> +    {
>>> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>>> +                     "%s: unable to get MTRR save record", __func__);
>>> +        goto out;
>>> +    }
>>> +
>>> +    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
>>> +
>>> +    /* TODO: maybe this should be a firmware option instead? */
>>> +    if ( !dom->device_model )
>>> +        /*
>>> +         * Enable MTRR, set default type to WB.
>>> +         * TODO: add MMIO areas as UC when passthrough is supported.
>>> +         */
>>> +        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
>>> +                                         MTRR_DEF_TYPE_ENABLE;
>> This is buggy.  MTRRs are per-vcpu and expected to match.  This only
>> works by chance in the HVM case because all settings are still 0 at this
>> point.
> Yes, I will look into it ASAP, but I'm not sure if the current
> save/restore trick that's done here will be possible with vcpus that
> are down or I will need to expand the save/restore logic to allow
> setting the state of vcpus that are down.

This gets back into "what is an offline vcpu" argument which happened
recently on the Introspection side.

As far as I'm concerned, once the vcpus have been allocated
XEN_DOMCTL_max_vcpus, they are present and "around".

Xen should be responsible for putting them into the correct INIT state
(which is one of the many bugs I've yet to get fully fixed from the
debug handling side of things), and the domain builder is responsible
for making any changes that firmware would normally expect to make.

Whether the vcpus are scheduled or not is unrelated to the state (and
indeed set-ability) of their registers.

~Andrew

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