Re: [PATCH v2 2/7] x86/vmx: add Intel PT MSR definitions

[Jan Beulich <jbeulich@suse.com>]

On 19.06.2020 01:39, Michał Leszczyński wrote:
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -621,4 +621,41 @@
>  #define MSR_PKGC9_IRTL			0x00000634
>  #define MSR_PKGC10_IRTL			0x00000635
>  
> +/* Intel PT MSRs */
> +#define MSR_RTIT_OUTPUT_BASE           0x00000560
> +#define MSR_RTIT_OUTPUT_MASK           0x00000561
> +#define MSR_RTIT_CTL                   0x00000570
> +#define RTIT_CTL_TRACEEN               (_AC(1, ULL) << 0)
> +#define RTIT_CTL_CYCEN                 (_AC(1, ULL) << 1)
> +#define RTIT_CTL_OS                    (_AC(1, ULL) << 2)
> +#define RTIT_CTL_USR                   (_AC(1, ULL) << 3)
> +#define RTIT_CTL_PWR_EVT_EN            (_AC(1, ULL) << 4)
> +#define RTIT_CTL_FUP_ON_PTW            (_AC(1, ULL) << 5)
> +#define RTIT_CTL_FABRIC_EN             (_AC(1, ULL) << 6)
> +#define RTIT_CTL_CR3_FILTER            (_AC(1, ULL) << 7)
> +#define RTIT_CTL_TOPA                  (_AC(1, ULL) << 8)
> +#define RTIT_CTL_MTC_EN                (_AC(1, ULL) << 9)
> +#define RTIT_CTL_TSC_EN                (_AC(1, ULL) << 10)
> +#define RTIT_CTL_DIS_RETC              (_AC(1, ULL) << 11)
> +#define RTIT_CTL_PTW_EN                (_AC(1, ULL) << 12)
> +#define RTIT_CTL_BRANCH_EN             (_AC(1, ULL) << 13)
> +#define RTIT_CTL_MTC_FREQ_OFFSET       14
> +#define RTIT_CTL_MTC_FREQ              (0x0fULL << RTIT_CTL_MTC_FREQ_OFFSET)

This was a fair step in the right direction, but you've missed
some instances (like here) wanting to use _AC(), and you've
also not moved the whole block up above the legacy line. As
Andrew's "x86/msr: Disallow access to Processor Trace MSRs" is
likely to go in before 4.14 in some form, you'll want to
re-base over it eventually anyway. You may want to take a look
there right away, to see where in the header to place your
addition.

If you look further up in the file you'll also notice how we
try to visually separate MSR numbers from bit-within-MSR
definitions.

Finally I'd also like to ask that you omit all RTIT_CTL_*_OFFSET
values. Only the _OFFSET-less #define-s should really be needed
- see MASK_EXTR() and MASK_INSR() in case right now you're using
these for some open-coded shifting ...

Jan