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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
Otherwise let's see what we can come to agree on.

Jan

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