x86, MCE, AMD: move invariant code out from loop body

PATCH In review Chen Yucong 2 participants 3 messages Oct 2, 2014 – Oct 7, 2014 3 messages not in the archive x86

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 1411377812.1917.112.camel@cyc

A message that is not in the archive (sent off-list or not yet received): [email protected]

Chen Yucong

author

2014-10-02 15:20 UTC #3 lore raw

PATCH 1/1 x86, MCE, AMD: move invariant code out from loop body

Quoted 20 lines · Borislav Petkov

Quoted 18 lines · Borislav Petkov
Quoted 17 lines · Chen Yucong
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;
                }
        }
}

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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 <[email protected]>
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 <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -1

index 5d4999f..f727701 100644
@@ -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;
 }
 
 /*

Signature · 1 line

1.7.10.4

Borislav Petkov

maintainer

2014-10-06 21:27 UTC #4 lore raw

Quoted 16 lines · Chen Yucong

From: Chen Yucong <[email protected]>
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 <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
index 5d4999f..f727701 100644

arch/x86/kernel/cpu/mcheck/mce_amd.c void mce_amd_feature_init(struct cpuinfo_x86 *c) · line 259

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -1

@@ -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):

arch/x86/kernel/cpu/mcheck/mce_amd.c +3 -1

index 9ce64955559d..9af7bd74828b 100644
@@ -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.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen Yucong

author

2014-10-07 06:10 UTC #5 lore raw

Quoted 19 lines · Borislav Petkov

Quoted 18 lines · Chen Yucong
From: Chen Yucong <[email protected]>
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 <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
index 5d4999f..f727701 100644
@@ -253,9 +253,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 			}
 

Quoted 11 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c void mce_amd_feature_init(struct cpuinfo_x86 *c) · line 259
Quoted 6 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -1
 			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.

Quoted 8 lines

Thus I think something like below should be much safer (I tried it with a label and goto already but it is uglier):
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
index 9ce64955559d..9af7bd74828b 100644

arch/x86/kernel/cpu/mcheck/mce_amd.c void mce_amd_feature_init(struct cpuinfo_x86 *c) · line 258

Quoted 7 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +3 -1

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

Quoted 13 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
 		}
 	}
 }
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

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +4 -1

index 5d4999f..31bf792 100644
@@ -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;
 		}
 	}
 }

Signature · 1 line

1.7.10.4