x86/msr: Start cleaning up msr-index.h
diff mbox series

Message ID 20200221151948.6209-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/msr: Start cleaning up msr-index.h
Related show

Commit Message

Andrew Cooper Feb. 21, 2020, 3:19 p.m. UTC
Make a start on cleaning up the constants in msr-index.h.

No functional change - only formatting changes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

Pulled out of a longer series which really ought to start making some
progress.
---
 xen/include/asm-x86/msr-index.h | 123 ++++++++++++++++++++++------------------
 1 file changed, 68 insertions(+), 55 deletions(-)

Comments

Jan Beulich Feb. 21, 2020, 3:41 p.m. UTC | #1
On 21.02.2020 16:19, Andrew Cooper wrote:
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -1,7 +1,74 @@
>  #ifndef __ASM_MSR_INDEX_H
>  #define __ASM_MSR_INDEX_H
>  
> -/* CPU model specific register (MSR) numbers */
> +/*
> + * CPU model specific register (MSR) numbers
> + *
> + * Definitions for an MSR should follow this style:
> + *
> + * #define MSR_$NAME                        0x$INDEX
> + * #define  $NAME_$BIT1                     (_AC(1, ULL) << $POS1)
> + * #define  $NAME_$BIT2                     (_AC(1, ULL) << $POS2)
> + *
> + * Blocks of related constants should be sorted by MSR index.  The constant
> + * names should be as concise as possible, and the bit names may have an
> + * abbreviated name.
> + */

Hmm, while "Blocks of related constants" caters for e.g. AMD's
MSR_AMD64_DR<n>_ADDRESS_MASK, I think for ease of lookup we
may want to be slightly more strict, without requiring strong
ordering. E.g. by also stating that multiple such blocks should
be ordered relative to one another also numerically (much like
we try to do in the insn emulator's huge switch() statement),
based on their lowest numbered MSR.

(As a nit, the example kind of implies that only single bit
fields would ever occur. It might avoid questions if you gave
a multi-bit example.)

> +#define MSR_APIC_BASE                       0x0000001b
> +#define  APIC_BASE_BSP                      (_AC(1, ULL) <<  8)
> +#define  APIC_BASE_EXTD                     (_AC(1, ULL) << 10)
> +#define  APIC_BASE_ENABLE                   (_AC(1, ULL) << 11)
> +#define  APIC_BASE_ADDR_MASK                0x000ffffffffff000ULL
> +
> +#define MSR_TEST_CTRL                       0x00000033
> +#define  TEST_CTRL_SPLITLOCK_DETECT         (_AC(1, ULL) << 29)
> +#define  TEST_CTRL_SPLITLOCK_DISABLE        (_AC(1, ULL) << 31)
> +
> +#define MSR_INTEL_CORE_THREAD_COUNT         0x00000035
> +#define  MSR_CTC_THREAD_MASK                0x0000ffff
> +#define  MSR_CTC_CORE_MASK                  0xffff0000
> +
> +#define MSR_SPEC_CTRL                       0x00000048
> +#define  SPEC_CTRL_IBRS                     (_AC(1, ULL) <<  0)
> +#define  SPEC_CTRL_STIBP                    (_AC(1, ULL) <<  1)
> +#define  SPEC_CTRL_SSBD                     (_AC(1, ULL) <<  2)
> +
> +#define MSR_PRED_CMD                        0x00000049
> +#define  PRED_CMD_IBPB                      (_AC(1, ULL) <<  0)
> +
> +#define MSR_PPIN_CTL                        0x0000004e
> +#define  PPIN_LOCKOUT                       (_AC(1, ULL) <<  0)
> +#define  PPIN_ENABLE                        (_AC(1, ULL) <<  1)
> +#define MSR_PPIN                            0x0000004f
> +
> +#define MSR_CORE_CAPABILITIES               0x000000cf
> +#define  CORE_CAPS_SPLITLOCK_DETECT         (_AC(1, ULL) <<  5)
> +
> +#define MSR_ARCH_CAPABILITIES               0x0000010a
> +#define  ARCH_CAPS_RDCL_NO                  (_AC(1, ULL) <<  0)
> +#define  ARCH_CAPS_IBRS_ALL                 (_AC(1, ULL) <<  1)
> +#define  ARCH_CAPS_RSBA                     (_AC(1, ULL) <<  2)
> +#define  ARCH_CAPS_SKIP_L1DFL               (_AC(1, ULL) <<  3)
> +#define  ARCH_CAPS_SSB_NO                   (_AC(1, ULL) <<  4)
> +#define  ARCH_CAPS_MDS_NO                   (_AC(1, ULL) <<  5)
> +#define  ARCH_CAPS_IF_PSCHANGE_MC_NO        (_AC(1, ULL) <<  6)
> +#define  ARCH_CAPS_TSX_CTRL                 (_AC(1, ULL) <<  7)
> +#define  ARCH_CAPS_TAA_NO                   (_AC(1, ULL) <<  8)
> +
> +#define MSR_FLUSH_CMD                       0x0000010b
> +#define  FLUSH_CMD_L1D                      (_AC(1, ULL) <<  0)
> +
> +#define MSR_TSX_FORCE_ABORT                 0x0000010f
> +#define  TSX_FORCE_ABORT_RTM                (_AC(1, ULL) <<  0)
> +
> +#define MSR_TSX_CTRL                        0x00000122
> +#define  TSX_CTRL_RTM_DISABLE               (_AC(1, ULL) <<  0)
> +#define  TSX_CTRL_CPUID_CLEAR               (_AC(1, ULL) <<  1)
> +
> +/*
> + * Legacy MSR constants in need of cleanup.  No new code below this comment.
> + */

"No new code" goes too far, I think. We shouldn't demand new
bits for existing MSRs to be accompanied by other cleanup of
that same MSR's definitions. "No new MSRs below ..." otoh
would be fine with me.

If you agree with both, feel free to add
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Otherwise let's see what we can come to agree on.

Jan
Andrew Cooper Feb. 21, 2020, 3:59 p.m. UTC | #2
On 21/02/2020 15:41, Jan Beulich wrote:
> On 21.02.2020 16:19, Andrew Cooper wrote:
>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -1,7 +1,74 @@
>>  #ifndef __ASM_MSR_INDEX_H
>>  #define __ASM_MSR_INDEX_H
>>  
>> -/* CPU model specific register (MSR) numbers */
>> +/*
>> + * CPU model specific register (MSR) numbers
>> + *
>> + * Definitions for an MSR should follow this style:
>> + *
>> + * #define MSR_$NAME                        0x$INDEX
>> + * #define  $NAME_$BIT1                     (_AC(1, ULL) << $POS1)
>> + * #define  $NAME_$BIT2                     (_AC(1, ULL) << $POS2)
>> + *
>> + * Blocks of related constants should be sorted by MSR index.  The constant
>> + * names should be as concise as possible, and the bit names may have an
>> + * abbreviated name.
>> + */
> Hmm, while "Blocks of related constants" caters for e.g. AMD's
> MSR_AMD64_DR<n>_ADDRESS_MASK, I think for ease of lookup we
> may want to be slightly more strict, without requiring strong
> ordering. E.g. by also stating that multiple such blocks should
> be ordered relative to one another also numerically (much like
> we try to do in the insn emulator's huge switch() statement),
> based on their lowest numbered MSR.

"Exceptions will be considered on a case-by-case basis" ?

It is not as if we'll ever be able to write down rules which will apply
uniformly to the whole file.

> (As a nit, the example kind of implies that only single bit
> fields would ever occur. It might avoid questions if you gave
> a multi-bit example.)

Single bit fields are the overwhelmingly common example.  Would
s/BIT/FIELD/ read any better, perhaps with $X and $Y instead of 1's in
the _AC() ?

>
>> +#define MSR_APIC_BASE                       0x0000001b
>> +#define  APIC_BASE_BSP                      (_AC(1, ULL) <<  8)
>> +#define  APIC_BASE_EXTD                     (_AC(1, ULL) << 10)
>> +#define  APIC_BASE_ENABLE                   (_AC(1, ULL) << 11)
>> +#define  APIC_BASE_ADDR_MASK                0x000ffffffffff000ULL
>> +
>> +#define MSR_TEST_CTRL                       0x00000033
>> +#define  TEST_CTRL_SPLITLOCK_DETECT         (_AC(1, ULL) << 29)
>> +#define  TEST_CTRL_SPLITLOCK_DISABLE        (_AC(1, ULL) << 31)
>> +
>> +#define MSR_INTEL_CORE_THREAD_COUNT         0x00000035
>> +#define  MSR_CTC_THREAD_MASK                0x0000ffff
>> +#define  MSR_CTC_CORE_MASK                  0xffff0000
>> +
>> +#define MSR_SPEC_CTRL                       0x00000048
>> +#define  SPEC_CTRL_IBRS                     (_AC(1, ULL) <<  0)
>> +#define  SPEC_CTRL_STIBP                    (_AC(1, ULL) <<  1)
>> +#define  SPEC_CTRL_SSBD                     (_AC(1, ULL) <<  2)
>> +
>> +#define MSR_PRED_CMD                        0x00000049
>> +#define  PRED_CMD_IBPB                      (_AC(1, ULL) <<  0)
>> +
>> +#define MSR_PPIN_CTL                        0x0000004e
>> +#define  PPIN_LOCKOUT                       (_AC(1, ULL) <<  0)
>> +#define  PPIN_ENABLE                        (_AC(1, ULL) <<  1)
>> +#define MSR_PPIN                            0x0000004f
>> +
>> +#define MSR_CORE_CAPABILITIES               0x000000cf
>> +#define  CORE_CAPS_SPLITLOCK_DETECT         (_AC(1, ULL) <<  5)
>> +
>> +#define MSR_ARCH_CAPABILITIES               0x0000010a
>> +#define  ARCH_CAPS_RDCL_NO                  (_AC(1, ULL) <<  0)
>> +#define  ARCH_CAPS_IBRS_ALL                 (_AC(1, ULL) <<  1)
>> +#define  ARCH_CAPS_RSBA                     (_AC(1, ULL) <<  2)
>> +#define  ARCH_CAPS_SKIP_L1DFL               (_AC(1, ULL) <<  3)
>> +#define  ARCH_CAPS_SSB_NO                   (_AC(1, ULL) <<  4)
>> +#define  ARCH_CAPS_MDS_NO                   (_AC(1, ULL) <<  5)
>> +#define  ARCH_CAPS_IF_PSCHANGE_MC_NO        (_AC(1, ULL) <<  6)
>> +#define  ARCH_CAPS_TSX_CTRL                 (_AC(1, ULL) <<  7)
>> +#define  ARCH_CAPS_TAA_NO                   (_AC(1, ULL) <<  8)
>> +
>> +#define MSR_FLUSH_CMD                       0x0000010b
>> +#define  FLUSH_CMD_L1D                      (_AC(1, ULL) <<  0)
>> +
>> +#define MSR_TSX_FORCE_ABORT                 0x0000010f
>> +#define  TSX_FORCE_ABORT_RTM                (_AC(1, ULL) <<  0)
>> +
>> +#define MSR_TSX_CTRL                        0x00000122
>> +#define  TSX_CTRL_RTM_DISABLE               (_AC(1, ULL) <<  0)
>> +#define  TSX_CTRL_CPUID_CLEAR               (_AC(1, ULL) <<  1)
>> +
>> +/*
>> + * Legacy MSR constants in need of cleanup.  No new code below this comment.
>> + */
> "No new code" goes too far, I think. We shouldn't demand new
> bits for existing MSRs to be accompanied by other cleanup of
> that same MSR's definitions. "No new MSRs below ..." otoh
> would be fine with me.

Ok.  TBH, I don't expect anyone but core maintainers to even know this
is here.  It is mainly a concrete separation between the two halves.

> If you agree with both, feel free to add
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, but I'd like your views on the suggestions above.

~Andrew
Jan Beulich Feb. 21, 2020, 4:03 p.m. UTC | #3
On 21.02.2020 16:59, Andrew Cooper wrote:
> On 21/02/2020 15:41, Jan Beulich wrote:
>> On 21.02.2020 16:19, Andrew Cooper wrote:
>>> --- a/xen/include/asm-x86/msr-index.h
>>> +++ b/xen/include/asm-x86/msr-index.h
>>> @@ -1,7 +1,74 @@
>>>  #ifndef __ASM_MSR_INDEX_H
>>>  #define __ASM_MSR_INDEX_H
>>>  
>>> -/* CPU model specific register (MSR) numbers */
>>> +/*
>>> + * CPU model specific register (MSR) numbers
>>> + *
>>> + * Definitions for an MSR should follow this style:
>>> + *
>>> + * #define MSR_$NAME                        0x$INDEX
>>> + * #define  $NAME_$BIT1                     (_AC(1, ULL) << $POS1)
>>> + * #define  $NAME_$BIT2                     (_AC(1, ULL) << $POS2)
>>> + *
>>> + * Blocks of related constants should be sorted by MSR index.  The constant
>>> + * names should be as concise as possible, and the bit names may have an
>>> + * abbreviated name.
>>> + */
>> Hmm, while "Blocks of related constants" caters for e.g. AMD's
>> MSR_AMD64_DR<n>_ADDRESS_MASK, I think for ease of lookup we
>> may want to be slightly more strict, without requiring strong
>> ordering. E.g. by also stating that multiple such blocks should
>> be ordered relative to one another also numerically (much like
>> we try to do in the insn emulator's huge switch() statement),
>> based on their lowest numbered MSR.
> 
> "Exceptions will be considered on a case-by-case basis" ?

Sounds good.

> It is not as if we'll ever be able to write down rules which will apply
> uniformly to the whole file.
> 
>> (As a nit, the example kind of implies that only single bit
>> fields would ever occur. It might avoid questions if you gave
>> a multi-bit example.)
> 
> Single bit fields are the overwhelmingly common example.  Would
> s/BIT/FIELD/ read any better, perhaps with $X and $Y instead of 1's in
> the _AC() ?

Yes.

Jan

Patch
diff mbox series

diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index c320846c06..c831cd2c60 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -1,7 +1,74 @@ 
 #ifndef __ASM_MSR_INDEX_H
 #define __ASM_MSR_INDEX_H
 
-/* CPU model specific register (MSR) numbers */
+/*
+ * CPU model specific register (MSR) numbers
+ *
+ * Definitions for an MSR should follow this style:
+ *
+ * #define MSR_$NAME                        0x$INDEX
+ * #define  $NAME_$BIT1                     (_AC(1, ULL) << $POS1)
+ * #define  $NAME_$BIT2                     (_AC(1, ULL) << $POS2)
+ *
+ * Blocks of related constants should be sorted by MSR index.  The constant
+ * names should be as concise as possible, and the bit names may have an
+ * abbreviated name.
+ */
+
+#define MSR_APIC_BASE                       0x0000001b
+#define  APIC_BASE_BSP                      (_AC(1, ULL) <<  8)
+#define  APIC_BASE_EXTD                     (_AC(1, ULL) << 10)
+#define  APIC_BASE_ENABLE                   (_AC(1, ULL) << 11)
+#define  APIC_BASE_ADDR_MASK                0x000ffffffffff000ULL
+
+#define MSR_TEST_CTRL                       0x00000033
+#define  TEST_CTRL_SPLITLOCK_DETECT         (_AC(1, ULL) << 29)
+#define  TEST_CTRL_SPLITLOCK_DISABLE        (_AC(1, ULL) << 31)
+
+#define MSR_INTEL_CORE_THREAD_COUNT         0x00000035
+#define  MSR_CTC_THREAD_MASK                0x0000ffff
+#define  MSR_CTC_CORE_MASK                  0xffff0000
+
+#define MSR_SPEC_CTRL                       0x00000048
+#define  SPEC_CTRL_IBRS                     (_AC(1, ULL) <<  0)
+#define  SPEC_CTRL_STIBP                    (_AC(1, ULL) <<  1)
+#define  SPEC_CTRL_SSBD                     (_AC(1, ULL) <<  2)
+
+#define MSR_PRED_CMD                        0x00000049
+#define  PRED_CMD_IBPB                      (_AC(1, ULL) <<  0)
+
+#define MSR_PPIN_CTL                        0x0000004e
+#define  PPIN_LOCKOUT                       (_AC(1, ULL) <<  0)
+#define  PPIN_ENABLE                        (_AC(1, ULL) <<  1)
+#define MSR_PPIN                            0x0000004f
+
+#define MSR_CORE_CAPABILITIES               0x000000cf
+#define  CORE_CAPS_SPLITLOCK_DETECT         (_AC(1, ULL) <<  5)
+
+#define MSR_ARCH_CAPABILITIES               0x0000010a
+#define  ARCH_CAPS_RDCL_NO                  (_AC(1, ULL) <<  0)
+#define  ARCH_CAPS_IBRS_ALL                 (_AC(1, ULL) <<  1)
+#define  ARCH_CAPS_RSBA                     (_AC(1, ULL) <<  2)
+#define  ARCH_CAPS_SKIP_L1DFL               (_AC(1, ULL) <<  3)
+#define  ARCH_CAPS_SSB_NO                   (_AC(1, ULL) <<  4)
+#define  ARCH_CAPS_MDS_NO                   (_AC(1, ULL) <<  5)
+#define  ARCH_CAPS_IF_PSCHANGE_MC_NO        (_AC(1, ULL) <<  6)
+#define  ARCH_CAPS_TSX_CTRL                 (_AC(1, ULL) <<  7)
+#define  ARCH_CAPS_TAA_NO                   (_AC(1, ULL) <<  8)
+
+#define MSR_FLUSH_CMD                       0x0000010b
+#define  FLUSH_CMD_L1D                      (_AC(1, ULL) <<  0)
+
+#define MSR_TSX_FORCE_ABORT                 0x0000010f
+#define  TSX_FORCE_ABORT_RTM                (_AC(1, ULL) <<  0)
+
+#define MSR_TSX_CTRL                        0x00000122
+#define  TSX_CTRL_RTM_DISABLE               (_AC(1, ULL) <<  0)
+#define  TSX_CTRL_CPUID_CLEAR               (_AC(1, ULL) <<  1)
+
+/*
+ * Legacy MSR constants in need of cleanup.  No new code below this comment.
+ */
 
 /* x86-64 specific MSRs */
 #define MSR_EFER		0xc0000080 /* extended feature register */
@@ -32,54 +99,6 @@ 
 #define EFER_KNOWN_MASK		(EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \
 				 EFER_SVME | EFER_FFXSE)
 
-#define MSR_TEST_CTRL                   0x00000033
-#define TEST_CTRL_SPLITLOCK_DETECT      (_AC(1, ULL) << 29)
-#define TEST_CTRL_SPLITLOCK_DISABLE     (_AC(1, ULL) << 31)
-
-#define MSR_INTEL_CORE_THREAD_COUNT     0x00000035
-#define MSR_CTC_THREAD_MASK             0x0000ffff
-#define MSR_CTC_CORE_MASK               0xffff0000
-
-/* Speculation Controls. */
-#define MSR_SPEC_CTRL			0x00000048
-#define SPEC_CTRL_IBRS			(_AC(1, ULL) << 0)
-#define SPEC_CTRL_STIBP			(_AC(1, ULL) << 1)
-#define SPEC_CTRL_SSBD			(_AC(1, ULL) << 2)
-
-#define MSR_PRED_CMD			0x00000049
-#define PRED_CMD_IBPB			(_AC(1, ULL) << 0)
-
-/* Intel Protected Processor Inventory Number */
-#define MSR_PPIN_CTL			0x0000004e
-#define MSR_PPIN			0x0000004f
-
-#define PPIN_LOCKOUT			(_AC(1, ULL) << 0)
-#define PPIN_ENABLE			(_AC(1, ULL) << 1)
-
-#define MSR_CORE_CAPABILITIES           0x000000cf
-#define CORE_CAPS_SPLITLOCK_DETECT      (_AC(1, ULL) <<  5)
-
-#define MSR_ARCH_CAPABILITIES		0x0000010a
-#define ARCH_CAPS_RDCL_NO		(_AC(1, ULL) << 0)
-#define ARCH_CAPS_IBRS_ALL		(_AC(1, ULL) << 1)
-#define ARCH_CAPS_RSBA			(_AC(1, ULL) << 2)
-#define ARCH_CAPS_SKIP_L1DFL		(_AC(1, ULL) << 3)
-#define ARCH_CAPS_SSB_NO		(_AC(1, ULL) << 4)
-#define ARCH_CAPS_MDS_NO		(_AC(1, ULL) << 5)
-#define ARCH_CAPS_IF_PSCHANGE_MC_NO	(_AC(1, ULL) << 6)
-#define ARCH_CAPS_TSX_CTRL		(_AC(1, ULL) << 7)
-#define ARCH_CAPS_TAA_NO		(_AC(1, ULL) << 8)
-
-#define MSR_FLUSH_CMD			0x0000010b
-#define FLUSH_CMD_L1D			(_AC(1, ULL) << 0)
-
-#define MSR_TSX_FORCE_ABORT             0x0000010f
-#define TSX_FORCE_ABORT_RTM             (_AC(1, ULL) <<  0)
-
-#define MSR_TSX_CTRL                    0x00000122
-#define TSX_CTRL_RTM_DISABLE            (_AC(1, ULL) <<  0)
-#define TSX_CTRL_CPUID_CLEAR            (_AC(1, ULL) <<  1)
-
 /* Intel MSRs. Some also available on other CPUs */
 #define MSR_IA32_PERFCTR0		0x000000c1
 #define MSR_IA32_A_PERFCTR0		0x000004c1
@@ -359,12 +378,6 @@ 
 
 #define MSR_IA32_TSC_ADJUST		0x0000003b
 
-#define MSR_APIC_BASE                   0x0000001b
-#define  APIC_BASE_BSP                  (1<<8)
-#define  APIC_BASE_EXTD                 (1<<10)
-#define  APIC_BASE_ENABLE               (1<<11)
-#define  APIC_BASE_ADDR_MASK            0x000ffffffffff000ul
-
 #define MSR_X2APIC_FIRST                0x00000800
 #define MSR_X2APIC_LAST                 0x00000bff