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 <jbeul...@suse.com> Thanks, but I'd like your views on the suggestions above. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
