Re: [PATCH v4 01/10] x86/vmx: add Intel PT MSR definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2020 14:33, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> 
> Define constants related to Intel Processor Trace features.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@xxxxxxx>

This needs re-basing onto current staging, now that Andrew's patch
to add the MSR numbers has gone in. Apart from this a couple of
cosmetic requests:

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -69,6 +69,43 @@
>  #define MSR_MCU_OPT_CTRL                    0x00000123
>  #define  MCU_OPT_CTRL_RNGDS_MITG_DIS        (_AC(1, ULL) <<  0)
>  
> +/* 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)

The right side is indented one space too many - see the similar
#define in context above.

> +#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)

The double blanks on the earlier lines exist such that here you
can reduce to a single one. You'll also find examples of this
further up in the file.

> +#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                   (_AC(0x0F, ULL) <<  14)

0xf please (i.e. lower case and no random number of leading
zeros).

> +#define  RTIT_CTL_CYC_THRESH                 (_AC(0x0F, ULL) <<  19)
> +#define  RTIT_CTL_PSB_FREQ                   (_AC(0x0F, ULL) <<  24)
> +#define  RTIT_CTL_ADDR(n)                    (_AC(0x0F, ULL) <<  (32 + (4 * 
> (n))))

Strictly speaking we don't need the parentheses around the operands
of binary * here - in mathematics precedence between + and * is
well defined. (We do parenthesize certain other expressions, when
the precedence may not be as well known.)

Thanks, Jan