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