[PATCH 0/9] x86/mtrr: basic cleanups

[PATCH 0/9] x86/mtrr: basic cleanups

[Doug Goldstein]

I was stuck on an airplane and as idly reading this code and noticed that
Xen does not need multiple MTRR implementations anymore since only x86_64
is supported. This guts some of the indirection and drops what should be
dead code paths. I will admit I have only compiled this and not booted it.

Doug Goldstein (9):
  x86/mtrr: prefix fns with mtrr and drop static
  x86/mtrr: drop mtrr_if indirection
  x86/mtrr: drop have_wrcomb() wrapper
  x86/mtrr: drop unnecessary use_intel() macro
  x86/mtrr: drop unused is_cpu() macro
  x86/mtrr: drop unused mtrr_ops struct
  x86/mtrr: drop unused positive_have_wrcomb()
  x86/mtrr: drop unused func prototypes and struct
  x86/mtrr: use stdbool instead of int + define

 xen/arch/x86/cpu/mtrr/generic.c   | 54 +++++++++++--------------------
 xen/arch/x86/cpu/mtrr/main.c      | 68 +++++++++++----------------------------
 xen/arch/x86/cpu/mtrr/mtrr.h      | 62 ++++++-----------------------------
 xen/arch/x86/platform_hypercall.c |  2 +-
 4 files changed, 48 insertions(+), 138 deletions(-)

-- 
2.7.3


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

[PATCH 1/9] x86/mtrr: prefix fns with mtrr and drop static

[Doug Goldstein]

For the functions that make up the interface to the MTRR code, drop the
static keyword and prefix them all with mtrr for improved clarity when
they're called outside this file. This also required adjusting or
providing function prototypes to make them callable.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/generic.c | 24 ++++++++++++------------
 xen/arch/x86/cpu/mtrr/mtrr.h    | 14 ++++++++++----
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 234d2ba..224d231 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -250,7 +250,7 @@ static void set_fixed_range(int msr, int * changed, unsigned int * msrwords)
 	}
 }
 
-int generic_get_free_region(unsigned long base, unsigned long size, int replace_reg)
+int mtrr_generic_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free MTRR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.
@@ -272,7 +272,7 @@ int generic_get_free_region(unsigned long base, unsigned long size, int replace_
 	return -ENOSPC;
 }
 
-static void generic_get_mtrr(unsigned int reg, unsigned long *base,
+void mtrr_generic_get(unsigned int reg, unsigned long *base,
 			     unsigned long *size, mtrr_type *type)
 {
 	uint64_t _mask, _base;
@@ -448,7 +448,7 @@ static void post_set(void)
 	spin_unlock(&set_atomicity_lock);
 }
 
-static void generic_set_all(void)
+void mtrr_generic_set_all(void)
 {
 	unsigned long mask, count;
 	unsigned long flags;
@@ -471,7 +471,7 @@ static void generic_set_all(void)
 	
 }
 
-static void generic_set_mtrr(unsigned int reg, unsigned long base,
+void mtrr_generic_set(unsigned int reg, unsigned long base,
 			     unsigned long size, mtrr_type type)
 /*  [SUMMARY] Set variable MTRR register on the local CPU.
     <reg> The register to set.
@@ -514,7 +514,7 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 	local_irq_restore(flags);
 }
 
-int generic_validate_add_page(unsigned long base, unsigned long size, unsigned int type)
+int mtrr_generic_validate_add_page(unsigned long base, unsigned long size, unsigned int type)
 {
 	unsigned long lbase, last;
 
@@ -549,7 +549,7 @@ int generic_validate_add_page(unsigned long base, unsigned long size, unsigned i
 }
 
 
-static int generic_have_wrcomb(void)
+int mtrr_generic_have_wrcomb(void)
 {
 	unsigned long config;
 	rdmsrl(MSR_MTRRcap, config);
@@ -565,10 +565,10 @@ int positive_have_wrcomb(void)
  */
 const struct mtrr_ops generic_mtrr_ops = {
 	.use_intel_if      = 1,
-	.set_all	   = generic_set_all,
-	.get               = generic_get_mtrr,
-	.get_free_region   = generic_get_free_region,
-	.set               = generic_set_mtrr,
-	.validate_add_page = generic_validate_add_page,
-	.have_wrcomb       = generic_have_wrcomb,
+	.set_all	   = mtrr_generic_set_all,
+	.get               = mtrr_generic_get,
+	.get_free_region   = mtrr_generic_get_free_region,
+	.set               = mtrr_generic_set,
+	.validate_add_page = mtrr_generic_validate_add_page,
+	.have_wrcomb       = mtrr_generic_have_wrcomb,
 };
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index b41eb58..619575f 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -29,10 +29,16 @@ struct mtrr_ops {
 	int	(*have_wrcomb)(void);
 };
 
-extern int generic_get_free_region(unsigned long base, unsigned long size,
-				   int replace_reg);
-extern int generic_validate_add_page(unsigned long base, unsigned long size,
-				     unsigned int type);
+void mtrr_generic_get(unsigned int reg, unsigned long *base,
+        unsigned long *size, mtrr_type *type);
+int mtrr_generic_get_free_region(unsigned long base, unsigned long size,
+        int replace_reg);
+int mtrr_generic_validate_add_page(unsigned long base, unsigned long size,
+        unsigned int type);
+void mtrr_generic_set_all(void);
+void mtrr_generic_set(unsigned int reg, unsigned long base,
+        unsigned long size, mtrr_type type);
+int mtrr_generic_have_wrcomb(void);
 
 extern const struct mtrr_ops generic_mtrr_ops;
 
-- 
2.7.3


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

[PATCH 2/9] x86/mtrr: drop mtrr_if indirection

[Doug Goldstein]

There can only ever be one mtrr_if now and that is the generic
implementation so instead of going through an indirect call change
everything to call the generic implementation directly. The is_cpu()
macro would result in the left side always being equal to
X86_VENDOR_INTEL for the generic implementation due to Intel having a
value of 0. The use_intel() macro was always true in the generic
implementation case as well. Removed some extraneous whitespace at
the same time.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/generic.c   |  2 +-
 xen/arch/x86/cpu/mtrr/main.c      | 47 ++++++++++++++-------------------------
 xen/arch/x86/cpu/mtrr/mtrr.h      |  4 ++--
 xen/arch/x86/platform_hypercall.c |  2 +-
 4 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 224d231..5c4b273 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -265,7 +265,7 @@ int mtrr_generic_get_free_region(unsigned long base, unsigned long size, int rep
 	if (replace_reg >= 0 && replace_reg < max)
 		return replace_reg;
 	for (i = 0; i < max; ++i) {
-		mtrr_if->get(i, &lbase, &lsize, &ltype);
+		mtrr_generic_get(i, &lbase, &lsize, &ltype);
 		if (lsize == 0)
 			return i;
 	}
diff --git a/xen/arch/x86/cpu/mtrr/main.c b/xen/arch/x86/cpu/mtrr/main.c
index bf489e3..ff908ad 100644
--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -58,8 +58,6 @@ static DEFINE_MUTEX(mtrr_mutex);
 u64 __read_mostly size_or_mask;
 u64 __read_mostly size_and_mask;
 
-const struct mtrr_ops *__read_mostly mtrr_if = NULL;
-
 static void set_mtrr(unsigned int reg, unsigned long base,
 		     unsigned long size, mtrr_type type);
 
@@ -82,7 +80,7 @@ static const char *mtrr_attrib_to_str(int x)
 /*  Returns non-zero if we have the write-combining memory type  */
 static int have_wrcomb(void)
 {
-	return (mtrr_if->have_wrcomb ? mtrr_if->have_wrcomb() : 0);
+	return mtrr_generic_have_wrcomb();
 }
 
 /*  This function returns the number of variable MTRRs  */
@@ -150,9 +148,9 @@ static void ipi_handler(void *info)
 	if (data->smp_reg == ~0U) /* update all mtrr registers */
 		/* At the cpu hot-add time this will reinitialize mtrr 
  		 * registres on the existing cpus. It is ok.  */
-		mtrr_if->set_all();
+		mtrr_generic_set_all();
 	else /* single mtrr register update */
-		mtrr_if->set(data->smp_reg, data->smp_base, 
+		mtrr_generic_set(data->smp_reg, data->smp_base,
 			     data->smp_size, data->smp_type);
 
 	atomic_dec(&data->count);
@@ -200,7 +198,7 @@ static inline int types_compatible(mtrr_type type1, mtrr_type type2) {
  * until it hits 0 and proceed. We set the data.gate flag and reset data.count.
  * Meanwhile, they are waiting for that flag to be set. Once it's set, each 
  * CPU goes through the transition of updating MTRRs. The CPU vendors may each do it 
- * differently, so we call mtrr_if->set() callback and let them take care of it.
+ * differently, so we call mtrr_generic_set() callback and let them take care of it.
  * When they're done, they again decrement data->count and wait for data.gate to 
  * be reset. 
  * When we finish, we wait for data.count to hit 0 and toggle the data.gate flag.
@@ -252,9 +250,9 @@ static void set_mtrr(unsigned int reg, unsigned long base,
 	if (reg == ~0U)  /* update all mtrr registers */
 		/* at boot or resume time, this will reinitialize the mtrrs on 
 		 * the bp. It is ok. */
-		mtrr_if->set_all();
+		mtrr_generic_set_all();
 	else /* update the single mtrr register */
-		mtrr_if->set(reg,base,size,type);
+		mtrr_generic_set(reg, base, size, type);
 
 	/* wait for the others */
 	while (atomic_read(&data.count))
@@ -317,10 +315,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,
 	mtrr_type ltype;
 	unsigned long lbase, lsize;
 
-	if (!mtrr_if)
-		return -ENXIO;
-		
-	if ((error = mtrr_if->validate_add_page(base,size,type)))
+	if ((error = mtrr_generic_validate_add_page(base,size,type)))
 		return error;
 
 	if (type >= MTRR_NUM_TYPES) {
@@ -351,7 +346,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,
 	/*  Search for existing MTRR  */
 	mutex_lock(&mtrr_mutex);
 	for (i = 0; i < num_var_ranges; ++i) {
-		mtrr_if->get(i, &lbase, &lsize, &ltype);
+		mtrr_generic_get(i, &lbase, &lsize, &ltype);
 		if (!lsize || base > lbase + lsize - 1 || base + size - 1 < lbase)
 			continue;
 		/*  At this point we know there is some kind of overlap/enclosure  */
@@ -386,7 +381,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,
 		goto out;
 	}
 	/*  Search for an empty MTRR  */
-	i = mtrr_if->get_free_region(base, size, replace);
+	i = mtrr_generic_get_free_region(base, size, replace);
 	if (i >= 0) {
 		set_mtrr(i, base, size, type);
 		if (likely(replace < 0))
@@ -487,15 +482,12 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)
 	unsigned long lbase, lsize;
 	int error = -EINVAL;
 
-	if (!mtrr_if)
-		return -ENXIO;
-
 	max = num_var_ranges;
 	mutex_lock(&mtrr_mutex);
 	if (reg < 0) {
 		/*  Search for existing MTRR  */
 		for (i = 0; i < max; ++i) {
-			mtrr_if->get(i, &lbase, &lsize, &ltype);
+			mtrr_generic_get(i, &lbase, &lsize, &ltype);
 			if (lbase == base && lsize == size) {
 				reg = i;
 				break;
@@ -511,7 +503,7 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)
 		printk(KERN_WARNING "mtrr: register: %d too big\n", reg);
 		goto out;
 	}
-	mtrr_if->get(reg, &lbase, &lsize, &ltype);
+	mtrr_generic_get(reg, &lbase, &lsize, &ltype);
 	if (lsize < 1) {
 		printk(KERN_WARNING "mtrr: MTRR %d not used\n", reg);
 		goto out;
@@ -569,22 +561,19 @@ struct mtrr_value {
 void __init mtrr_bp_init(void)
 {
 	if (cpu_has_mtrr) {
-		mtrr_if = &generic_mtrr_ops;
 		size_or_mask = ~((1ULL << (paddr_bits - PAGE_SHIFT)) - 1);
 		size_and_mask = ~size_or_mask & 0xfffff00000ULL;
 	}
 
-	if (mtrr_if) {
-		set_num_var_ranges();
-		init_table();
-		if (use_intel())
-			get_mtrr_state();
-	}
+    set_num_var_ranges();
+    init_table();
+    if (use_intel())
+        get_mtrr_state();
 }
 
 void mtrr_ap_init(void)
 {
-	if (!mtrr_if || !use_intel() || hold_mtrr_updates_on_aps)
+	if (!use_intel() || hold_mtrr_updates_on_aps)
 		return;
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries changed,
@@ -630,13 +619,11 @@ void mtrr_bp_restore(void)
 {
 	if (!use_intel())
 		return;
-	mtrr_if->set_all();
+	mtrr_generic_set_all();
 }
 
 static int __init mtrr_init_finialize(void)
 {
-	if (!mtrr_if)
-		return 0;
 	if (use_intel())
 		mtrr_state_warn();
 	return 0;
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 619575f..92b0b11 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -63,8 +63,8 @@ extern void set_mtrr_ops(const struct mtrr_ops *);
 extern u64 size_or_mask, size_and_mask;
 extern const struct mtrr_ops *mtrr_if;
 
-#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
-#define use_intel()	(mtrr_if && mtrr_if->use_intel_if == 1)
+#define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
+#define use_intel()	(1)
 
 extern unsigned int num_var_ranges;
 
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 780f22d..4a294b4 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -265,7 +265,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
         ret = -EINVAL;
         if ( op->u.read_memtype.reg < num_var_ranges )
         {
-            mtrr_if->get(op->u.read_memtype.reg, &mfn, &nr_mfns, &type);
+            mtrr_generic_get(op->u.read_memtype.reg, &mfn, &nr_mfns, &type);
             op->u.read_memtype.mfn     = mfn;
             op->u.read_memtype.nr_mfns = nr_mfns;
             op->u.read_memtype.type    = type;
-- 
2.7.3


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

[PATCH 3/9] x86/mtrr: drop have_wrcomb() wrapper

[Doug Goldstein]

The only call was always to the generic implementation.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/main.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/main.c b/xen/arch/x86/cpu/mtrr/main.c
index ff908ad..5dd1f5d 100644
--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -77,12 +77,6 @@ static const char *mtrr_attrib_to_str(int x)
 	return (x <= 6) ? mtrr_strings[x] : "?";
 }
 
-/*  Returns non-zero if we have the write-combining memory type  */
-static int have_wrcomb(void)
-{
-	return mtrr_generic_have_wrcomb();
-}
-
 /*  This function returns the number of variable MTRRs  */
 static void __init set_num_var_ranges(void)
 {
@@ -324,7 +318,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,
 	}
 
 	/*  If the type is WC, check that this processor supports it  */
-	if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
+	if ((type == MTRR_TYPE_WRCOMB) && !mtrr_generic_have_wrcomb()) {
 		printk(KERN_WARNING
 		       "mtrr: your processor doesn't support write-combining\n");
 		return -ENOSYS;
-- 
2.7.3


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

[PATCH 4/9] x86/mtrr: drop unnecessary use_intel() macro

[Doug Goldstein]

The use_intel() macro always evaluates to true so don't bother using it.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/main.c | 21 ++++-----------------
 xen/arch/x86/cpu/mtrr/mtrr.h |  1 -
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/main.c b/xen/arch/x86/cpu/mtrr/main.c
index 5dd1f5d..6f0113a 100644
--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -82,12 +82,7 @@ static void __init set_num_var_ranges(void)
 {
 	unsigned long config = 0;
 
-	if (use_intel()) {
-		rdmsrl(MSR_MTRRcap, config);
-	} else if (is_cpu(AMD))
-		config = 2;
-	else if (is_cpu(CYRIX) || is_cpu(CENTAUR))
-		config = 8;
+    rdmsrl(MSR_MTRRcap, config);
 	num_var_ranges = config & 0xff;
 }
 
@@ -561,13 +556,12 @@ void __init mtrr_bp_init(void)
 
     set_num_var_ranges();
     init_table();
-    if (use_intel())
-        get_mtrr_state();
+    get_mtrr_state();
 }
 
 void mtrr_ap_init(void)
 {
-	if (!use_intel() || hold_mtrr_updates_on_aps)
+	if (hold_mtrr_updates_on_aps)
 		return;
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries changed,
@@ -596,30 +590,23 @@ void mtrr_save_state(void)
 
 void mtrr_aps_sync_begin(void)
 {
-	if (!use_intel())
-		return;
 	hold_mtrr_updates_on_aps = 1;
 }
 
 void mtrr_aps_sync_end(void)
 {
-	if (!use_intel())
-		return;
 	set_mtrr(~0U, 0, 0, 0);
 	hold_mtrr_updates_on_aps = 0;
 }
 
 void mtrr_bp_restore(void)
 {
-	if (!use_intel())
-		return;
 	mtrr_generic_set_all();
 }
 
 static int __init mtrr_init_finialize(void)
 {
-	if (use_intel())
-		mtrr_state_warn();
+    mtrr_state_warn();
 	return 0;
 }
 __initcall(mtrr_init_finialize);
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 92b0b11..5e0d832 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -64,7 +64,6 @@ extern u64 size_or_mask, size_and_mask;
 extern const struct mtrr_ops *mtrr_if;
 
 #define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
-#define use_intel()	(1)
 
 extern unsigned int num_var_ranges;
 
-- 
2.7.3


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

[PATCH 5/9] x86/mtrr: drop unused is_cpu() macro

[Doug Goldstein]

is_cpu() evaluated to Intel always so just drop it entirely.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/generic.c | 2 +-
 xen/arch/x86/cpu/mtrr/mtrr.h    | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 5c4b273..45d4def 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -520,7 +520,7 @@ int mtrr_generic_validate_add_page(unsigned long base, unsigned long size, unsig
 
 	/*  For Intel PPro stepping <= 7, must be 4 MiB aligned 
 	    and not touch 0x70000000->0x7003FFFF */
-	if (is_cpu(INTEL) && boot_cpu_data.x86 == 6 &&
+	if (boot_cpu_data.x86 == 6 &&
 	    boot_cpu_data.x86_model == 1 &&
 	    boot_cpu_data.x86_mask <= 7) {
 		if (base & ((1 << (22 - PAGE_SHIFT)) - 1)) {
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 5e0d832..25f4867 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -63,8 +63,6 @@ extern void set_mtrr_ops(const struct mtrr_ops *);
 extern u64 size_or_mask, size_and_mask;
 extern const struct mtrr_ops *mtrr_if;
 
-#define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
-
 extern unsigned int num_var_ranges;
 
 void mtrr_state_warn(void);
-- 
2.7.3


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

[PATCH 6/9] x86/mtrr: drop unused mtrr_ops struct

[Doug Goldstein]

There are no users of the mtrr_ops struct or any of the callers on it so
drop those.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/generic.c | 12 ------------
 xen/arch/x86/cpu/mtrr/mtrr.h    | 23 -----------------------
 2 files changed, 35 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 45d4def..1d67035 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -560,15 +560,3 @@ int positive_have_wrcomb(void)
 {
 	return 1;
 }
-
-/* generic structure...
- */
-const struct mtrr_ops generic_mtrr_ops = {
-	.use_intel_if      = 1,
-	.set_all	   = mtrr_generic_set_all,
-	.get               = mtrr_generic_get,
-	.get_free_region   = mtrr_generic_get_free_region,
-	.set               = mtrr_generic_set,
-	.validate_add_page = mtrr_generic_validate_add_page,
-	.have_wrcomb       = mtrr_generic_have_wrcomb,
-};
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 25f4867..9391fc5 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -11,24 +11,6 @@
 #define MTRR_CHANGE_MASK_VARIABLE  0x02
 #define MTRR_CHANGE_MASK_DEFTYPE   0x04
 
-
-struct mtrr_ops {
-	u32	vendor;
-	u32	use_intel_if;
-//	void	(*init)(void);
-	void	(*set)(unsigned int reg, unsigned long base,
-		       unsigned long size, mtrr_type type);
-	void	(*set_all)(void);
-
-	void	(*get)(unsigned int reg, unsigned long *base,
-		       unsigned long *size, mtrr_type * type);
-	int	(*get_free_region)(unsigned long base, unsigned long size,
-				   int replace_reg);
-	int	(*validate_add_page)(unsigned long base, unsigned long size,
-				     unsigned int type);
-	int	(*have_wrcomb)(void);
-};
-
 void mtrr_generic_get(unsigned int reg, unsigned long *base,
         unsigned long *size, mtrr_type *type);
 int mtrr_generic_get_free_region(unsigned long base, unsigned long size,
@@ -40,8 +22,6 @@ void mtrr_generic_set(unsigned int reg, unsigned long base,
         unsigned long size, mtrr_type type);
 int mtrr_generic_have_wrcomb(void);
 
-extern const struct mtrr_ops generic_mtrr_ops;
-
 extern int positive_have_wrcomb(void);
 
 /* library functions for processor-specific routines */
@@ -58,10 +38,7 @@ void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
 
 void get_mtrr_state(void);
 
-extern void set_mtrr_ops(const struct mtrr_ops *);
-
 extern u64 size_or_mask, size_and_mask;
-extern const struct mtrr_ops *mtrr_if;
 
 extern unsigned int num_var_ranges;
 
-- 
2.7.3


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

[PATCH 7/9] x86/mtrr: drop unused positive_have_wrcomb()

[Doug Goldstein]

Unused function, gone.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/generic.c | 5 -----
 xen/arch/x86/cpu/mtrr/mtrr.h    | 2 --
 2 files changed, 7 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 1d67035..012aca4 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -555,8 +555,3 @@ int mtrr_generic_have_wrcomb(void)
 	rdmsrl(MSR_MTRRcap, config);
 	return (config & (1ULL << 10));
 }
-
-int positive_have_wrcomb(void)
-{
-	return 1;
-}
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 9391fc5..9405cbc 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -22,8 +22,6 @@ void mtrr_generic_set(unsigned int reg, unsigned long base,
         unsigned long size, mtrr_type type);
 int mtrr_generic_have_wrcomb(void);
 
-extern int positive_have_wrcomb(void);
-
 /* library functions for processor-specific routines */
 struct set_mtrr_context {
 	unsigned long flags;
-- 
2.7.3


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

[PATCH 8/9] x86/mtrr: drop unused func prototypes and struct

[Doug Goldstein]

These weren't used so drop them.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/mtrr.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 9405cbc..1a3b1e5 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -22,18 +22,6 @@ void mtrr_generic_set(unsigned int reg, unsigned long base,
         unsigned long size, mtrr_type type);
 int mtrr_generic_have_wrcomb(void);
 
-/* library functions for processor-specific routines */
-struct set_mtrr_context {
-	unsigned long flags;
-	unsigned long cr4val;
-	uint64_t deftype;
-	u32 ccr3;
-};
-
-void set_mtrr_done(struct set_mtrr_context *ctxt);
-void set_mtrr_cache_disable(struct set_mtrr_context *ctxt);
-void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
-
 void get_mtrr_state(void);
 
 extern u64 size_or_mask, size_and_mask;
@@ -41,6 +29,3 @@ extern u64 size_or_mask, size_and_mask;
 extern unsigned int num_var_ranges;
 
 void mtrr_state_warn(void);
-
-extern int amd_init_mtrr(void);
-extern int cyrix_init_mtrr(void);
-- 
2.7.3


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

[PATCH 9/9] x86/mtrr: use stdbool instead of int + define

[Doug Goldstein]

Instead of using an int and providing a define for TRUE and FALSE,
change the code to use stdbool that Xen provides.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 xen/arch/x86/cpu/mtrr/generic.c | 21 +++++++++++----------
 xen/arch/x86/cpu/mtrr/mtrr.h    |  5 -----
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 012aca4..2d2eadc 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -3,6 +3,7 @@
 #include <xen/lib.h>
 #include <xen/init.h>
 #include <xen/mm.h>
+#include <xen/stdbool.h>
 #include <asm/flushtlb.h>
 #include <asm/io.h>
 #include <asm/mtrr.h>
@@ -237,7 +238,7 @@ static void mtrr_wrmsr(unsigned int msr, uint64_t msr_content)
  * \param changed pointer which indicates whether the MTRR needed to be changed
  * \param msrwords pointer to the MSR values which the MSR should have
  */
-static void set_fixed_range(int msr, int * changed, unsigned int * msrwords)
+static void set_fixed_range(int msr, bool * changed, unsigned int * msrwords)
 {
 	uint64_t msr_content, val;
 
@@ -246,7 +247,7 @@ static void set_fixed_range(int msr, int * changed, unsigned int * msrwords)
 
 	if (msr_content != val) {
 		mtrr_wrmsr(msr, val);
-		*changed = TRUE;
+		*changed = true;
 	}
 }
 
@@ -302,10 +303,10 @@ void mtrr_generic_get(unsigned int reg, unsigned long *base,
  * Checks and updates the fixed-range MTRRs if they differ from the saved set
  * \param frs pointer to fixed-range MTRR values, saved by get_fixed_ranges()
  */
-static int set_fixed_ranges(mtrr_type * frs)
+static bool set_fixed_ranges(mtrr_type * frs)
 {
 	unsigned long long *saved = (unsigned long long *) frs;
-	int changed = FALSE;
+	bool changed = false;
 	int block=-1, range;
 
 	while (fixed_range_blocks[++block].ranges)
@@ -316,13 +317,13 @@ static int set_fixed_ranges(mtrr_type * frs)
 	return changed;
 }
 
-/*  Set the MSR pair relating to a var range. Returns TRUE if
+/*  Set the MSR pair relating to a var range. Returns true if
     changes are made  */
-static int set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
+static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
 {
 	uint32_t lo, hi, base_lo, base_hi, mask_lo, mask_hi;
 	uint64_t msr_content;
-	int changed = FALSE;
+	bool changed = false;
 
 	rdmsrl(MSR_IA32_MTRR_PHYSBASE(index), msr_content);
 	lo = (uint32_t)msr_content;
@@ -337,7 +338,7 @@ static int set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
 
 	if ((base_lo != lo) || (base_hi != hi)) {
 		mtrr_wrmsr(MSR_IA32_MTRR_PHYSBASE(index), vr->base);
-		changed = TRUE;
+		changed = true;
 	}
 
 	rdmsrl(MSR_IA32_MTRR_PHYSMASK(index), msr_content);
@@ -353,7 +354,7 @@ static int set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
 
 	if ((mask_lo != lo) || (mask_hi != hi)) {
 		mtrr_wrmsr(MSR_IA32_MTRR_PHYSMASK(index), vr->mask);
-		changed = TRUE;
+		changed = true;
 	}
 	return changed;
 }
@@ -478,7 +479,7 @@ void mtrr_generic_set(unsigned int reg, unsigned long base,
     <base> The base address of the region.
     <size> The size of the region. If this is 0 the region is disabled.
     <type> The type of the region.
-    <do_safe> If TRUE, do the change safely. If FALSE, safety measures should
+    <do_safe> If true, do the change safely. If false, safety measures should
     be done externally.
     [RETURNS] Nothing.
 */
diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index 1a3b1e5..9d55c68 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -2,11 +2,6 @@
  * local mtrr defines.
  */
 
-#ifndef TRUE
-#define TRUE  1
-#define FALSE 0
-#endif
-
 #define MTRR_CHANGE_MASK_FIXED     0x01
 #define MTRR_CHANGE_MASK_VARIABLE  0x02
 #define MTRR_CHANGE_MASK_DEFTYPE   0x04
-- 
2.7.3


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

Re: [PATCH 1/9] x86/mtrr: prefix fns with mtrr and drop static

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> For the functions that make up the interface to the MTRR code, drop the
> static keyword and prefix them all with mtrr for improved clarity when
> they're called outside this file. This also required adjusting or
> providing function prototypes to make them callable.

I can see why you want to do this for non-static functions, but I can't
see why static ones would need to get altered (unless you mean to call
them directly in subsequent patches, in which case the rationale above
should be adjusted). Nor can I see why the two functions previously
having been non-static can't simply be made static, without changing
their names, as the patch demonstrates that they don't have callers
in other CUs.

Jan


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

Re: [PATCH 2/9] x86/mtrr: drop mtrr_if indirection

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> There can only ever be one mtrr_if now and that is the generic
> implementation

This is only true when taking into consideration that cpu_has_mtrr
is #define-d to 1 right now. I'm not sure that's actually a good
assumption (especially when think about running Xen itself
virtualized, or possibly adding a mode of operation where no MTRRs
are to be used). But if we want to keep it that way, then I'd suggest
this patch should include removing cpu_has_mtrr (which will then
show to the reviewers that the checks of mtrr_if against NULL
indeed are dead code.

> @@ -569,22 +561,19 @@ struct mtrr_value {
>  void __init mtrr_bp_init(void)
>  {
>  	if (cpu_has_mtrr) {
> -		mtrr_if = &generic_mtrr_ops;
>  		size_or_mask = ~((1ULL << (paddr_bits - PAGE_SHIFT)) - 1);
>  		size_and_mask = ~size_or_mask & 0xfffff00000ULL;
>  	}
>  
> -	if (mtrr_if) {
> -		set_num_var_ranges();
> -		init_table();
> -		if (use_intel())
> -			get_mtrr_state();
> -	}
> +    set_num_var_ranges();
> +    init_table();
> +    if (use_intel())
> +        get_mtrr_state();
>  }

Please don't break indentation style.

> --- a/xen/arch/x86/cpu/mtrr/mtrr.h
> +++ b/xen/arch/x86/cpu/mtrr/mtrr.h
> @@ -63,8 +63,8 @@ extern void set_mtrr_ops(const struct mtrr_ops *);
>  extern u64 size_or_mask, size_and_mask;
>  extern const struct mtrr_ops *mtrr_if;
>  
> -#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
> -#define use_intel()	(mtrr_if && mtrr_if->use_intel_if == 1)
> +#define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
> +#define use_intel()	(1)

Is the latter really useful to keep then?

Jan


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

Re: [PATCH 3/9] x86/mtrr: drop have_wrcomb() wrapper

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> The only call was always to the generic implementation.
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 4/9] x86/mtrr: drop unnecessary use_intel() macro

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> The use_intel() macro always evaluates to true so don't bother using it.

Ah, here we go. But there are indentation issues again here.

Jan


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

Re: [PATCH 5/9] x86/mtrr: drop unused is_cpu() macro

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -520,7 +520,7 @@ int mtrr_generic_validate_add_page(unsigned long base, unsigned long size, unsig
>  
>  	/*  For Intel PPro stepping <= 7, must be 4 MiB aligned 
>  	    and not touch 0x70000000->0x7003FFFF */
> -	if (is_cpu(INTEL) && boot_cpu_data.x86 == 6 &&
> +	if (boot_cpu_data.x86 == 6 &&
>  	    boot_cpu_data.x86_model == 1 &&
>  	    boot_cpu_data.x86_mask <= 7) {
>  		if (base & ((1 << (22 - PAGE_SHIFT)) - 1)) {

Well, this check then was and is (a) wrong (regardless of the MTRR
flavor used the family/model/stepping check has to be CPU vendor
specific) and (b) dead code (we don't run on PPro-s anymore).
Please instead delete it altogether.

Jan


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

Re: [PATCH 6/9] x86/mtrr: drop unused mtrr_ops struct

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> There are no users of the mtrr_ops struct or any of the callers on it so
> drop those.
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 7/9] x86/mtrr: drop unused positive_have_wrcomb()

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> Unused function, gone.
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

But perhaps worth getting folded into the one dropping have_wrcomb()
(the acks for both patches can be retained for the merged result).

Jan


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

Re: [PATCH 8/9] x86/mtrr: drop unused func prototypes and struct

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> These weren't used so drop them.
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 9/9] x86/mtrr: use stdbool instead of int + define

[Jan Beulich]

>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -3,6 +3,7 @@
>  #include <xen/lib.h>
>  #include <xen/init.h>
>  #include <xen/mm.h>
> +#include <xen/stdbool.h>

Is this really needed, with xen/types.h already including it?

> @@ -237,7 +238,7 @@ static void mtrr_wrmsr(unsigned int msr, uint64_t msr_content)
>   * \param changed pointer which indicates whether the MTRR needed to be changed
>   * \param msrwords pointer to the MSR values which the MSR should have
>   */
> -static void set_fixed_range(int msr, int * changed, unsigned int * msrwords)
> +static void set_fixed_range(int msr, bool * changed, unsigned int * msrwords)

Please drop the stray blanks while touching this.

> @@ -478,7 +479,7 @@ void mtrr_generic_set(unsigned int reg, unsigned long base,
>      <base> The base address of the region.
>      <size> The size of the region. If this is 0 the region is disabled.
>      <type> The type of the region.
> -    <do_safe> If TRUE, do the change safely. If FALSE, safety measures should
> +    <do_safe> If true, do the change safely. If false, safety measures should
>      be done externally.

Please drop this stale comment portion instead of adjusting it.

Jan


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

Re: [PATCH 1/9] x86/mtrr: prefix fns with mtrr and drop static

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 1021 bytes --]

On 8/17/16 7:36 AM, Jan Beulich wrote:
>>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
>> For the functions that make up the interface to the MTRR code, drop the
>> static keyword and prefix them all with mtrr for improved clarity when
>> they're called outside this file. This also required adjusting or
>> providing function prototypes to make them callable.
> 
> I can see why you want to do this for non-static functions, but I can't
> see why static ones would need to get altered (unless you mean to call
> them directly in subsequent patches, in which case the rationale above
> should be adjusted). Nor can I see why the two functions previously
> having been non-static can't simply be made static, without changing
> their names, as the patch demonstrates that they don't have callers
> in other CUs.
> 
> Jan
> 

I've added:

Future changes will directly call these functions instead of using the
indirect call through the ops structure.

Does that work?

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 2/9] x86/mtrr: drop mtrr_if indirection

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 2205 bytes --]

On 8/17/16 7:49 AM, Jan Beulich wrote:
>>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
>> There can only ever be one mtrr_if now and that is the generic
>> implementation
> 
> This is only true when taking into consideration that cpu_has_mtrr
> is #define-d to 1 right now. I'm not sure that's actually a good
> assumption (especially when think about running Xen itself
> virtualized, or possibly adding a mode of operation where no MTRRs
> are to be used). But if we want to keep it that way, then I'd suggest
> this patch should include removing cpu_has_mtrr (which will then
> show to the reviewers that the checks of mtrr_if against NULL
> indeed are dead code.

Sure I can remove cpu_has_mtrr that would certainly make it cleaner. Is
it ok with you and Andrew to make the assumption that we'll always have
MTRRs (until the day we don't like you described)?

> 
>> @@ -569,22 +561,19 @@ struct mtrr_value {
>>  void __init mtrr_bp_init(void)
>>  {
>>  	if (cpu_has_mtrr) {
>> -		mtrr_if = &generic_mtrr_ops;
>>  		size_or_mask = ~((1ULL << (paddr_bits - PAGE_SHIFT)) - 1);
>>  		size_and_mask = ~size_or_mask & 0xfffff00000ULL;
>>  	}
>>  
>> -	if (mtrr_if) {
>> -		set_num_var_ranges();
>> -		init_table();
>> -		if (use_intel())
>> -			get_mtrr_state();
>> -	}
>> +    set_num_var_ranges();
>> +    init_table();
>> +    if (use_intel())
>> +        get_mtrr_state();
>>  }
> 
> Please don't break indentation style.

Sad panda. This file has tabs. Sorry I missed that.

> 
>> --- a/xen/arch/x86/cpu/mtrr/mtrr.h
>> +++ b/xen/arch/x86/cpu/mtrr/mtrr.h
>> @@ -63,8 +63,8 @@ extern void set_mtrr_ops(const struct mtrr_ops *);
>>  extern u64 size_or_mask, size_and_mask;
>>  extern const struct mtrr_ops *mtrr_if;
>>  
>> -#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
>> -#define use_intel()	(mtrr_if && mtrr_if->use_intel_if == 1)
>> +#define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
>> +#define use_intel()	(1)
> 
> Is the latter really useful to keep then?

Would you like me to squash patch 4 into this or make a note in the
commit message that further clean ups are coming?

--
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/9] x86/mtrr: prefix fns with mtrr and drop static

[Jan Beulich]

>>> On 18.08.16 at 03:38, <cardoe@cardoe.com> wrote:
> On 8/17/16 7:36 AM, Jan Beulich wrote:
>>>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
>>> For the functions that make up the interface to the MTRR code, drop the
>>> static keyword and prefix them all with mtrr for improved clarity when
>>> they're called outside this file. This also required adjusting or
>>> providing function prototypes to make them callable.
>> 
>> I can see why you want to do this for non-static functions, but I can't
>> see why static ones would need to get altered (unless you mean to call
>> them directly in subsequent patches, in which case the rationale above
>> should be adjusted). Nor can I see why the two functions previously
>> having been non-static can't simply be made static, without changing
>> their names, as the patch demonstrates that they don't have callers
>> in other CUs.
> 
> I've added:
> 
> Future changes will directly call these functions instead of using the
> indirect call through the ops structure.
> 
> Does that work?

Sure, having seen the further parts of the series.

Jan


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

Re: [PATCH 2/9] x86/mtrr: drop mtrr_if indirection

[Jan Beulich]

>>> On 18.08.16 at 03:59, <cardoe@cardoe.com> wrote:
> On 8/17/16 7:49 AM, Jan Beulich wrote:
>>>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
>>> There can only ever be one mtrr_if now and that is the generic
>>> implementation
>> 
>> This is only true when taking into consideration that cpu_has_mtrr
>> is #define-d to 1 right now. I'm not sure that's actually a good
>> assumption (especially when think about running Xen itself
>> virtualized, or possibly adding a mode of operation where no MTRRs
>> are to be used). But if we want to keep it that way, then I'd suggest
>> this patch should include removing cpu_has_mtrr (which will then
>> show to the reviewers that the checks of mtrr_if against NULL
>> indeed are dead code.
> 
> Sure I can remove cpu_has_mtrr that would certainly make it cleaner. Is
> it ok with you and Andrew to make the assumption that we'll always have
> MTRRs (until the day we don't like you described)?

Well, that's what I've basically put up for discussion. My personal
preference would be to drop the hard coding of cpu_has_mtrr (and
hence keep it as well as mtrr_if).

>>> --- a/xen/arch/x86/cpu/mtrr/mtrr.h
>>> +++ b/xen/arch/x86/cpu/mtrr/mtrr.h
>>> @@ -63,8 +63,8 @@ extern void set_mtrr_ops(const struct mtrr_ops *);
>>>  extern u64 size_or_mask, size_and_mask;
>>>  extern const struct mtrr_ops *mtrr_if;
>>>  
>>> -#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
>>> -#define use_intel()	(mtrr_if && mtrr_if->use_intel_if == 1)
>>> +#define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
>>> +#define use_intel()	(1)
>> 
>> Is the latter really useful to keep then?
> 
> Would you like me to squash patch 4 into this or make a note in the
> commit message that further clean ups are coming?

Either way is fine with me, all I wish is for it to be clear that you
don't stop half ways with the cleanup (which I'm glad you took a
stab on, btw).

Jan


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

Re: [PATCH 2/9] x86/mtrr: drop mtrr_if indirection

[Andrew Cooper]

On 18/08/16 02:59, Doug Goldstein wrote:
> On 8/17/16 7:49 AM, Jan Beulich wrote:
>>>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
>>> There can only ever be one mtrr_if now and that is the generic
>>> implementation
>> This is only true when taking into consideration that cpu_has_mtrr
>> is #define-d to 1 right now. I'm not sure that's actually a good
>> assumption (especially when think about running Xen itself
>> virtualized, or possibly adding a mode of operation where no MTRRs
>> are to be used). But if we want to keep it that way, then I'd suggest
>> this patch should include removing cpu_has_mtrr (which will then
>> show to the reviewers that the checks of mtrr_if against NULL
>> indeed are dead code.
> Sure I can remove cpu_has_mtrr that would certainly make it cleaner. Is
> it ok with you and Andrew to make the assumption that we'll always have
> MTRRs (until the day we don't like you described)?

Please don't remove cpu_has_mtrr.  I plan to make better use of it with
the future plan to move PV guests into a PVH container.

In such a case, the PVH container won't want/need MTRRs, and we will get
better performance by not having them available.

~Andrew

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