Re: [Xen-devel] [PATCH] x86/msr: Start cleaning up msr-index.h

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel