From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Huang Subject: Re: [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR Date: Tue, 5 Apr 2011 12:26:42 -0500 Message-ID: <4D9B50D2.8040900@amd.com> References: Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------060207080108070809030407" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "'xen-devel@lists.xensource.com'" List-Id: xen-devel@lists.xenproject.org --------------060207080108070809030407 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit 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" 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 >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > > --------------060207080108070809030407 Content-Type: text/plain; name="amd_fix_syscfg.txt" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="amd_fix_syscfg.txt" Content-Description: amd_fix_syscfg.txt # HG changeset patch # User Wei Huang # 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 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 = { --------------060207080108070809030407 Content-Type: text/plain; name="amd_remove_k8_enable_fixed_iorrs.txt" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="amd_remove_k8_enable_fixed_iorrs.txt" Content-Description: amd_remove_k8_enable_fixed_iorrs.txt # HG changeset patch # User Wei Huang # 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 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; } --------------060207080108070809030407 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------060207080108070809030407--