* [PATCH] x86, MCE, AMD: move invariant code out from loop body [not found] ` <20141002143819.GE16452@pd.tnic> @ 2014-10-02 15:20 ` Chen Yucong 2014-10-06 21:27 ` Borislav Petkov 0 siblings, 1 reply; 3+ messages in thread From: Chen Yucong @ 2014-10-02 15:20 UTC (permalink / raw) To: Borislav Petkov; +Cc: Tony Luck, linux-edac, linux-kernel On Thu, 2014-10-02 at 16:38 +0200, Borislav Petkov wrote: > > On Mon, Sep 22, 2014 at 09:11:00PM +0200, Borislav Petkov wrote: > > On Mon, Sep 22, 2014 at 05:23:32PM +0800, Chen Yucong wrote: > > > Hi Boris, > > > > > > I have found the following code snippet in mce_amd.c. > > > > > > /* cpu init entry point, called from mce.c with preempt off */ > > > void mce_amd_feature_init(struct cpuinfo_x86 *c) > > > { > > > ... ... > > > for (bank = 0; bank < mca_cfg.banks; ++bank) { > > > for (block = 0; block < NR_BLOCKS; ++block) { > > > ... ... > > > mce_threshold_block_init(&b, offset); > > > mce_threshold_vector = amd_threshold_interrupt; > > > } > > > } > > > } > > > > > > Why should "mce_threshold_vector = amd_threshold_interrupt" be placed in > > > the inner loop body? > > > > Yeah, it was added sloppily with b276268631af3, I'm not surprised. Feel > > free to send a fix. > > do you still want to send a fix or should I fix it up quickly? > From: Chen Yucong <slaoub@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 <slaoub@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; } /* -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] x86, MCE, AMD: move invariant code out from loop body 2014-10-02 15:20 ` [PATCH] x86, MCE, AMD: move invariant code out from loop body Chen Yucong @ 2014-10-06 21:27 ` Borislav Petkov 2014-10-07 6:08 ` Chen Yucong 0 siblings, 1 reply; 3+ messages in thread From: Borislav Petkov @ 2014-10-06 21:27 UTC (permalink / raw) To: Chen Yucong; +Cc: Tony Luck, linux-edac, linux-kernel On Thu, Oct 02, 2014 at 11:20:12PM +0800, Chen Yucong wrote: > From: Chen Yucong <slaoub@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 <slaoub@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. 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; } } } 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... Hmmm. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] x86, MCE, AMD: move invariant code out from loop body 2014-10-06 21:27 ` Borislav Petkov @ 2014-10-07 6:08 ` Chen Yucong 0 siblings, 0 replies; 3+ messages in thread From: Chen Yucong @ 2014-10-07 6:08 UTC (permalink / raw) To: Borislav Petkov; +Cc: Tony Luck, linux-edac, linux-kernel 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 <slaoub@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 <slaoub@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 <slaoub@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 <slaoub@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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-07 6:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1411377812.1917.112.camel@cyc>
[not found] ` <20140922191100.GC4709@pd.tnic>
[not found] ` <20141002143819.GE16452@pd.tnic>
2014-10-02 15:20 ` [PATCH] x86, MCE, AMD: move invariant code out from loop body Chen Yucong
2014-10-06 21:27 ` Borislav Petkov
2014-10-07 6:08 ` Chen Yucong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).