On Mon, 2014-10-06 at 23:27 +0200, Borislav Petkov wrote: > On Thu, Oct 02, 2014 at 11:20:12PM +0800, Chen Yucong wrote: > > From: Chen Yucong <sla...@gmail.com> > > Subject: [PATCH] x86, MCE, AMD: move invariant code out from loop body > > > > "mce_threshold_vector = amd_threshold_interrupt;" is loop invariant code > > in mce_amd_feature_init(). So it should be moved out from loop body. > > > > Signed-off-by: Chen Yucong <sla...@gmail.com> > > --- > > arch/x86/kernel/cpu/mcheck/mce_amd.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c > > b/arch/x86/kernel/cpu/mcheck/mce_amd.c > > index 5d4999f..f727701 100644 > > --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c > > +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c > > @@ -253,9 +253,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c) > > } > > > > mce_threshold_block_init(&b, offset); > > - mce_threshold_vector = amd_threshold_interrupt; > > } > > } > > + > > + mce_threshold_vector = amd_threshold_interrupt; > > Looking at this more, it is theoretically possible that we break out > of the both loops without *any* thresholding registers detected and to > still assign a thresholding interrupt vector which would be clearly > wrong. Yes! In this case, mce_threshold_vector should be `default_threshold_interrupt' rather than amd_threshold_interrupt. > Thus I think something like below should be much safer (I tried it with > a label and goto already but it is uglier): > > diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c > b/arch/x86/kernel/cpu/mcheck/mce_amd.c > index 9ce64955559d..9af7bd74828b 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c > +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c > @@ -253,7 +253,9 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c) > } > > mce_threshold_block_init(&b, offset); > - mce_threshold_vector = amd_threshold_interrupt; > + > + if (mce_threshold_vector != amd_threshold_interrupt) > + mce_threshold_vector = amd_threshold_interrupt; Perhaps the above assignment operation should be put into
if (b.interrupt_capable) { ... ... if (mce_threshold_vector != amd_threshold_interrupt) mce_threshold_vector = amd_threshold_interrupt; } If IntP (Thresholding Interrupt Supported) bit is zero, this indicates that the reporting of threshold overflow via interrupt isn't supported. So there's no need to execute the above assignment operation. > } > } > } > > Looking at the asm, we still go and fetch those addresses so not really > a win: > > cmpq $amd_threshold_interrupt, mce_threshold_vector(%rip) #, > mce_threshold_vector > je .L235 #, > incl %r13d # block > movq $amd_threshold_interrupt, mce_threshold_vector(%rip) #, > mce_threshold_vector > cmpl $9, %r13d #, block > > but this way the code is relatively clean. Unless you can come up with > a nicer, cleaner version to handle the breaking out in the success and > failure case... Seems like I don't have any better idea than this. thx! cyc From: Chen Yucong <sla...@gmail.com> Subject: [PATCH] x86, MCE, AMD: avoid inappropriate assignment operation in mce_amd_feature_init Before executing "mce_threshold_vector = amd_threshold_interrupt;", a few conditions should be checked for avoiding inappropriate assignment operations, for example, IntP (Thresholding Interrupt Supported) bit of MCx_MISCi. Signed-off-by: Chen Yucong <sla...@gmail.com> --- arch/x86/kernel/cpu/mcheck/mce_amd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c index 5d4999f..31bf792 100644 --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c @@ -250,10 +250,13 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c) if (b.interrupt_capable) { int new = (high & MASK_LVTOFF_HI) >> 20; offset = setup_APIC_mce(offset, new); + + if (offset == new && + mce_threshold_vector != amd_threshold_interrupt) + mce_threshold_vector = amd_threshold_interrupt; } mce_threshold_block_init(&b, offset); - mce_threshold_vector = amd_threshold_interrupt; } } } -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/