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

[Wei Huang <wei.huang2@amd.com>]

[-- 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