Wei Huang
2011-Apr-04 22:23 UTC
[Xen-devel] [PATCH] MTRR: clear 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. The patch was derived from upstream kernel patch (see https://patchwork.kernel.org/patch/11425/). Signed-off-by: Wei Huang <wei.huang2@amd.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Apr-05 11:51 UTC
Re: [Xen-devel] [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Huang
2011-Apr-05 17:26 UTC
Re: [Xen-devel] [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR
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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Apr-05 18:12 UTC
Re: [Xen-devel] [PATCH] MTRR: clear DramModEn bit of sys_cfg MSR
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 >> >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel