[PATCH] MTRR: clear DramModEn bit of sys_cfg MSR

[PATCH] MTRR: clear DramModEn bit of sys_cfg MSR

[Wei Huang]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

Some buggy BIOS might set sys_cfg DramModEn bit to 1, which can cause 
unexpected behavior on AMD platforms. This patch clears DramModEn bit. 
The patch was derived from upstream kernel patch (see 
https://patchwork.kernel.org/patch/11425/).

Signed-off-by: Wei Huang <wei.huang2@amd.com>





[-- Attachment #2: amd_fix_syscfg_dram_mod_en.txt --]
[-- Type: text/plain, Size: 3027 bytes --]

# HG changeset patch
# User Wei Huang <wei.huang2@amd.com>
# Date 1301955822 18000
# Node ID e9ab70bf676a89e5c2f3380622c335ee8f55ede3
# Parent  5a10b635262c0d5816415f616ade21264fc53f64
MTRR: clear AMD SYS_CFG DramModEn to 0

Some buggy BIOS might set SYS_CFG DramModEn bit to 1, which can cause unexpected behavior on AMD platforms. This patch clears DramModEn bit if it is 1. The patch was derived from upstream kernel patch (see https://patchwork.kernel.org/patch/11425/).

Signed-off-by: Wei Huang <wei.huang2@amd.com>

diff -r 5a10b635262c -r e9ab70bf676a xen/arch/x86/cpu/mtrr/generic.c
--- a/xen/arch/x86/cpu/mtrr/generic.c	Mon Apr 04 13:29:38 2011 -0500
+++ b/xen/arch/x86/cpu/mtrr/generic.c	Mon Apr 04 17:23:42 2011 -0500
@@ -34,11 +34,36 @@
 	rdmsrl(MTRRphysMask_MSR(index), vr->mask);
 }
 
+/*
+ * BIOS is expected to clear MtrrFixDramModEn bit. According to AMD BKDG : 
+ * "The MtrrFixDramModEn bit should be set to 1 during BIOS initalization of 
+ * the fixed MTRRs, then cleared to 0 for operation."
+ */
+static inline void amd_check_syscfg_dram_mod_en(void)
+{
+    uint64_t syscfg;
+
+    if (!((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
+          (boot_cpu_data.x86 >= 0x0f)))
+        return;
+
+    rdmsrl(MSR_K8_SYSCFG, syscfg);
+    if (syscfg & K8_MTRRFIXRANGE_DRAM_MODIFY) {
+        printk(KERN_ERR "MTRR: CPU %u: SYSCFG[MtrrFixDramModEn]"
+               " not cleared by BIOS, clearing this bit\n",
+               smp_processor_id());
+        syscfg &= ~K8_MTRRFIXRANGE_DRAM_MODIFY;
+        mtrr_wrmsr(MSR_K8_SYSCFG, syscfg);
+    }
+}
+
 static void
 get_fixed_ranges(mtrr_type * frs)
 {
 	uint64_t *p = (uint64_t *) frs;
 	int i;
+
+        amd_check_syscfg_dram_mod_en();
 
 	rdmsrl(MTRRfix64K_00000_MSR, p[0]);
 
@@ -116,20 +141,6 @@
 }
 
 /**
- * Enable and allow read/write of extended fixed-range MTRR bits on K8 CPUs
- * see AMD publication no. 24593, chapter 3.2.1 for more information
- */
-static inline void k8_enable_fixed_iorrs(void)
-{
-	uint64_t msr_content;
-
-	rdmsrl(MSR_K8_SYSCFG, msr_content);
-	mtrr_wrmsr(MSR_K8_SYSCFG, msr_content 
-				| K8_MTRRFIXRANGE_DRAM_ENABLE
-				| K8_MTRRFIXRANGE_DRAM_MODIFY);
-}
-
-/**
  * Checks and updates an fixed-range MTRR if it differs from the value it
  * should have. If K8 extenstions are wanted, update the K8 SYSCFG MSR also.
  * see AMD publication no. 24593, chapter 7.8.1, page 233 for more information
@@ -145,10 +156,6 @@
 	val = ((uint64_t)msrwords[1] << 32) | msrwords[0];
 
 	if (msr_content != val) {
-		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
-		    boot_cpu_data.x86 == 15 &&
-		    ((msrwords[0] | msrwords[1]) & K8_MTRR_RDMEM_WRMEM_MASK))
-			k8_enable_fixed_iorrs();
 		mtrr_wrmsr(msr, val);
 		*changed = TRUE;
 	}
@@ -211,6 +218,8 @@
 	unsigned long long *saved = (unsigned long long *) frs;
 	int changed = FALSE;
 	int block=-1, range;
+
+        amd_check_syscfg_dram_mod_en();
 
 	while (fixed_range_blocks[++block].ranges)
 	    for (range=0; range < fixed_range_blocks[block].ranges; range++)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR

[Keir Fraser]

On 04/04/2011 23:23, "Wei Huang" <wei.huang2@amd.com> wrote:

> Some buggy BIOS might set sys_cfg DramModEn bit to 1, which can cause
> unexpected behavior on AMD platforms. This patch clears DramModEn bit.
> The patch was derived from upstream kernel patch (see
> https://patchwork.kernel.org/patch/11425/).

This patch also removes k8_enable_fixed_iorrs(), and it's not clear why.
That would at least belong in a separate patch, but I would think we don't
want to delete that code anyway.

The indentation is wrong (spaces in a file that is hard-tab-indented). And
the printk is probably unnecessary -- at most you should print it once
rather than potentially for every core brought up.

But further, I don't see why you need to hang off {get,set}_fixed_ranges at
all. Why not do this check-and-fixup in cpu/amd.c:init_amd()? It's a handy
early-cpu-bringup amd-specific function which is rather designed ofr this
kind of purpose. The k8_enable_fixed_iorrs() work could better be done in
the same place, too (perhaps move it in a separate patch?).

 -- Keir

> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR

[Wei Huang]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

Sorry for the indention issue... I agree that it is better to move 
check-and_fixup code to init_amd() function. See the new patch.

My understanding is that RdMem and WrMem are never going to become 1: 
BIOS are supposed to set sys_cfg[DramModEn] bit to 0 before OS takes 
over. As a result, RdMem and WrMem are read as 0 in fixed MTRRs. Unless 
Xen turn these bits to 1 (which I don't see from the code), 
k8_enable_fixed_iorrs() is useless. My 2nd patch removes 
k8_enable_fixed_iorrs(). If you have concern, we don't have to apply 
this patch. But I don't think we shouldn't move it to amd.c. 
k8_enable_fixed_iorrs() is unrelated in that file.

Thanks,
-Wei

On 04/05/2011 06:51 AM, Keir Fraser wrote:
> On 04/04/2011 23:23, "Wei Huang"<wei.huang2@amd.com>  wrote:
>
>> Some buggy BIOS might set sys_cfg DramModEn bit to 1, which can cause
>> unexpected behavior on AMD platforms. This patch clears DramModEn bit.
>> The patch was derived from upstream kernel patch (see
>> https://patchwork.kernel.org/patch/11425/).
> This patch also removes k8_enable_fixed_iorrs(), and it's not clear why.
> That would at least belong in a separate patch, but I would think we don't
> want to delete that code anyway.
>
> The indentation is wrong (spaces in a file that is hard-tab-indented). And
> the printk is probably unnecessary -- at most you should print it once
> rather than potentially for every core brought up.
>
> But further, I don't see why you need to hang off {get,set}_fixed_ranges at
> all. Why not do this check-and-fixup in cpu/amd.c:init_amd()? It's a handy
> early-cpu-bringup amd-specific function which is rather designed ofr this
> kind of purpose. The k8_enable_fixed_iorrs() work could better be done in
> the same place, too (perhaps move it in a separate patch?).
>
>   -- Keir
>
>> Signed-off-by: Wei Huang<wei.huang2@amd.com>
>>
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>


[-- Attachment #2: amd_fix_syscfg.txt --]
[-- Type: text/plain, Size: 1624 bytes --]

# HG changeset patch
# User Wei Huang <wei.huang2@amd.com>
# Date 1302023015 18000
# Node ID 4a9430efe643d18c8b69d377a6e70325ac7a9483
# Parent  8957b6b3f43932b4e50f489bc50ed87ab1a286cd
MTRR: correct DramModEn bit of SYS_CFG MSR

Some buggy BIOS might set SYS_CFG DramModEn bit to 1, which can cause unexpected behavior on AMD platforms. This patch clears DramModEn bit if it is 1.

Signed-off-by: Wei Huang <wei.huang2@amd.com>

diff -r 8957b6b3f439 -r 4a9430efe643 xen/arch/x86/cpu/amd.c
--- a/xen/arch/x86/cpu/amd.c	Tue Apr 05 11:13:02 2011 -0500
+++ b/xen/arch/x86/cpu/amd.c	Tue Apr 05 12:03:35 2011 -0500
@@ -300,6 +300,32 @@
 		on_each_cpu(disable_c1e, NULL, 1);
 }
 
+/*
+ * BIOS is expected to clear MtrrFixDramModEn bit. According to AMD BKDG : 
+ * "The MtrrFixDramModEn bit should be set to 1 during BIOS initalization of 
+ * the fixed MTRRs, then cleared to 0 for operation."
+ */
+static void check_syscfg_dram_mod_en(void)
+{
+	uint64_t syscfg;
+	static int printed = 0;
+
+	if (!((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
+		(boot_cpu_data.x86 >= 0x0f)))
+		return;
+
+	rdmsrl(MSR_K8_SYSCFG, syscfg);
+	if (syscfg & K8_MTRRFIXRANGE_DRAM_MODIFY) {
+		if (!printed) {
+			printed++;
+			printk(KERN_ERR "MTRR: SYSCFG[MtrrFixDramModEn] not "
+				"cleared by BIOS, clearing this bit\n");
+		}
+		syscfg &= ~K8_MTRRFIXRANGE_DRAM_MODIFY;
+		wrmsrl(MSR_K8_SYSCFG, syscfg);
+	}
+}
+
 static void __devinit init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -453,6 +479,8 @@
 		disable_c1_ramping();
 
 	set_cpuidmask(c);
+
+	check_syscfg_dram_mod_en();
 }
 
 static struct cpu_dev amd_cpu_dev __cpuinitdata = {

[-- Attachment #3: amd_remove_k8_enable_fixed_iorrs.txt --]
[-- Type: text/plain, Size: 1873 bytes --]

# HG changeset patch
# User Wei Huang <wei.huang2@amd.com>
# Date 1302023857 18000
# Node ID effc147d35ce692c5d55d007a722869c372963c1
# Parent  4a9430efe643d18c8b69d377a6e70325ac7a9483
MTRR: remove k8_enable_fixed_iorrs()

AMD64 defines two special bits (bit 3 and 4) RdMem and WrMem in fixed MTRR type. Their values are supposed to be 0 after BIOS hands the control to OS according to AMD BKDG. Unless OS specificially turn them on, they are kept 0 all the time. As a result, k8_enable_fixed_iorrs() is unnecessary and removed from upstream kernel (see https://patchwork.kernel.org/patch/11425/). This patch does the same thing.

Signed-off-by: Wei Huang <wei.huang2@amd.com>

diff -r 4a9430efe643 -r effc147d35ce xen/arch/x86/cpu/mtrr/generic.c
--- a/xen/arch/x86/cpu/mtrr/generic.c	Tue Apr 05 12:03:35 2011 -0500
+++ b/xen/arch/x86/cpu/mtrr/generic.c	Tue Apr 05 12:17:37 2011 -0500
@@ -116,20 +116,6 @@
 }
 
 /**
- * Enable and allow read/write of extended fixed-range MTRR bits on K8 CPUs
- * see AMD publication no. 24593, chapter 3.2.1 for more information
- */
-static inline void k8_enable_fixed_iorrs(void)
-{
-	uint64_t msr_content;
-
-	rdmsrl(MSR_K8_SYSCFG, msr_content);
-	mtrr_wrmsr(MSR_K8_SYSCFG, msr_content 
-				| K8_MTRRFIXRANGE_DRAM_ENABLE
-				| K8_MTRRFIXRANGE_DRAM_MODIFY);
-}
-
-/**
  * Checks and updates an fixed-range MTRR if it differs from the value it
  * should have. If K8 extenstions are wanted, update the K8 SYSCFG MSR also.
  * see AMD publication no. 24593, chapter 7.8.1, page 233 for more information
@@ -145,10 +131,6 @@
 	val = ((uint64_t)msrwords[1] << 32) | msrwords[0];
 
 	if (msr_content != val) {
-		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
-		    boot_cpu_data.x86 == 15 &&
-		    ((msrwords[0] | msrwords[1]) & K8_MTRR_RDMEM_WRMEM_MASK))
-			k8_enable_fixed_iorrs();
 		mtrr_wrmsr(msr, val);
 		*changed = TRUE;
 	}

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR

[Keir Fraser]

Looks good, thanks!

 -- Keir

On 05/04/2011 18:26, "Wei Huang" <wei.huang2@amd.com> wrote:

> Sorry for the indention issue... I agree that it is better to move
> check-and_fixup code to init_amd() function. See the new patch.
> 
> My understanding is that RdMem and WrMem are never going to become 1:
> BIOS are supposed to set sys_cfg[DramModEn] bit to 0 before OS takes
> over. As a result, RdMem and WrMem are read as 0 in fixed MTRRs. Unless
> Xen turn these bits to 1 (which I don't see from the code),
> k8_enable_fixed_iorrs() is useless. My 2nd patch removes
> k8_enable_fixed_iorrs(). If you have concern, we don't have to apply
> this patch. But I don't think we shouldn't move it to amd.c.
> k8_enable_fixed_iorrs() is unrelated in that file.
> 
> Thanks,
> -Wei
> 
> On 04/05/2011 06:51 AM, Keir Fraser wrote:
>> On 04/04/2011 23:23, "Wei Huang"<wei.huang2@amd.com>  wrote:
>> 
>>> Some buggy BIOS might set sys_cfg DramModEn bit to 1, which can cause
>>> unexpected behavior on AMD platforms. This patch clears DramModEn bit.
>>> The patch was derived from upstream kernel patch (see
>>> https://patchwork.kernel.org/patch/11425/).
>> This patch also removes k8_enable_fixed_iorrs(), and it's not clear why.
>> That would at least belong in a separate patch, but I would think we don't
>> want to delete that code anyway.
>> 
>> The indentation is wrong (spaces in a file that is hard-tab-indented). And
>> the printk is probably unnecessary -- at most you should print it once
>> rather than potentially for every core brought up.
>> 
>> But further, I don't see why you need to hang off {get,set}_fixed_ranges at
>> all. Why not do this check-and-fixup in cpu/amd.c:init_amd()? It's a handy
>> early-cpu-bringup amd-specific function which is rather designed ofr this
>> kind of purpose. The k8_enable_fixed_iorrs() work could better be done in
>> the same place, too (perhaps move it in a separate patch?).
>> 
>>   -- Keir
>> 
>>> Signed-off-by: Wei Huang<wei.huang2@amd.com>
>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>> 
>> 
>