[2/2] x86/mce: fix logic and comments around MSR_PPIN_CTL

Commit Message

Jan Beulich March 2, 2020, 2:07 p.m. UTC

From: Tony Luck <[e-mail]>

There are two implemented bits in the PPIN_CTL MSR:

Bit0: LockOut (R/WO)
      Set 1 to prevent further writes to MSR_PPIN_CTL.

Bit 1: Enable_PPIN (R/W)
       If 1, enables MSR_PPIN to be accessible using RDMSR.
       If 0, an attempt to read MSR_PPIN will cause #GP.

So there are four defined values:
	0: PPIN is disabled, PPIN_CTL may be updated
	1: PPIN is disabled. PPIN_CTL is locked against updates
	2: PPIN is enabled. PPIN_CTL may be updated
	3: PPIN is enabled. PPIN_CTL is locked against updates

Code would only enable the X86_FEATURE_INTEL_PPIN feature for case "2".
When it should have done so for both case "2" and case "3".

Fix the final test to just check for the enable bit.
Also fix some of the other comments in this function.

Signed-off-by: Tony Luck <[e-mail]>
[Linux commit ???]

One of the adjusted comments doesn't exist in our code, and I disagree
with the adjustment to the other one and its associate code change: I
don't think there's a point trying to enable PPIN if the locked bit is
set. Hence it's just the main code change that gets pulled in, plus it
gets cloned to the AMD side.

Requested-by: Andrew Cooper <[e-mail]>
Signed-off-by: Jan Beulich <[e-mail]>
---
Note: If available by the time of committing I mean to insert the Linux
      commit hash.

Comments

Andrew Cooper March 2, 2020, 2:26 p.m. UTC | #1

On 02/03/2020 14:07, Jan Beulich wrote:
> From: Tony Luck <[e-mail]>
>
> There are two implemented bits in the PPIN_CTL MSR:
>
> Bit0: LockOut (R/WO)
>       Set 1 to prevent further writes to MSR_PPIN_CTL.
>
> Bit 1: Enable_PPIN (R/W)
>        If 1, enables MSR_PPIN to be accessible using RDMSR.
>        If 0, an attempt to read MSR_PPIN will cause #GP.
>
> So there are four defined values:
> 	0: PPIN is disabled, PPIN_CTL may be updated
> 	1: PPIN is disabled. PPIN_CTL is locked against updates
> 	2: PPIN is enabled. PPIN_CTL may be updated
> 	3: PPIN is enabled. PPIN_CTL is locked against updates
>
> Code would only enable the X86_FEATURE_INTEL_PPIN feature for case "2".
> When it should have done so for both case "2" and case "3".
>
> Fix the final test to just check for the enable bit.
> Also fix some of the other comments in this function.
>
> Signed-off-by: Tony Luck <[e-mail]>
> [Linux commit ???]
>
> One of the adjusted comments doesn't exist in our code, and I disagree
> with the adjustment to the other one and its associate code change: I
> don't think there's a point trying to enable PPIN if the locked bit is
> set. Hence it's just the main code change that gets pulled in, plus it
> gets cloned to the AMD side.
>
> Requested-by: Andrew Cooper <[e-mail]>
> Signed-off-by: Jan Beulich <[e-mail]>

I agree.  If it is locked, there is no point trying to change it.

Reviewed-by: Andrew Cooper <[e-mail]>

Patch

--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -885,7 +885,7 @@  static void intel_init_ppin(const struct
             rdmsr_safe(MSR_PPIN_CTL, val);
         }
 
-        if ( (val & (PPIN_ENABLE | PPIN_LOCKOUT)) != PPIN_ENABLE )
+        if ( !(val & PPIN_ENABLE) )
             ppin_msr = 0;
         else if ( c == &boot_cpu_data )
             ppin_msr = MSR_PPIN;
--- a/xen/arch/x86/cpu/mcheck/mce_amd.c
+++ b/xen/arch/x86/cpu/mcheck/mce_amd.c
@@ -329,7 +329,7 @@  amd_mcheck_init(struct cpuinfo_x86 *ci)
             rdmsrl(MSR_AMD_PPIN_CTL, val);
         }
 
-        if ( (val & (PPIN_ENABLE | PPIN_LOCKOUT)) != PPIN_ENABLE )
+        if ( !(val & PPIN_ENABLE) )
             ppin_msr = 0;
         else if ( ci == &boot_cpu_data )
             ppin_msr = MSR_AMD_PPIN;