[v4,01/10] x86/vmx: add Intel PT MSR definitions
diff mbox series

Message ID 2ff9ecee8367e814a29b17a34203bda0e3c48d74.1593519420.git.michal.leszczynski@cert.pl
State Superseded
Headers show
Series
  • Implement support for external IPT monitoring
Related show

Commit Message

Michał Leszczyński June 30, 2020, 12:33 p.m. UTC
From: Michal Leszczynski <michal.leszczynski@cert.pl>

Define constants related to Intel Processor Trace features.

Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
---
 xen/include/asm-x86/msr-index.h | 37 +++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Jan Beulich June 30, 2020, 4:23 p.m. UTC | #1
On 30.06.2020 14:33, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@cert.pl>
> 
> Define constants related to Intel Processor Trace features.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>

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
Andrew Cooper June 30, 2020, 5:37 p.m. UTC | #2
On 30/06/2020 13:33, Michał Leszczyński wrote:
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index b328a47ed8..0203029be9 100644
> --- 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)
> +#define  RTIT_CTL_CYCEN                      (_AC(1, ULL) <<  1)

In addition to what Jan has said, please can we be consistent with an
underscore (or not) before EN.  Preferably with, so these would become
TRACE_EN and CYC_EN.

That said, there are a lot of bit definitions which aren't used at all. 
IMO, it would be better to introduce defines when you use them.

Thanks,

~Andrew
Tamas K Lengyel June 30, 2020, 6:03 p.m. UTC | #3
On Tue, Jun 30, 2020 at 11:39 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 30/06/2020 13:33, Michał Leszczyński wrote:
> > diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> > index b328a47ed8..0203029be9 100644
> > --- 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)
> > +#define  RTIT_CTL_CYCEN                      (_AC(1, ULL) <<  1)
>
> In addition to what Jan has said, please can we be consistent with an
> underscore (or not) before EN.  Preferably with, so these would become
> TRACE_EN and CYC_EN.
>
> That said, there are a lot of bit definitions which aren't used at all.
> IMO, it would be better to introduce defines when you use them.

In the past I found it very valuable when this type of plumbing was
already present in Xen instead of me having to go into the SDM to digg
out the magic numbers. So while some of the bits might not be used
right now I also don't see any downside in having them, just in case.

Tamas
Michał Leszczyński June 30, 2020, 6:27 p.m. UTC | #4
----- 30 cze 2020 o 20:03, Tamas K Lengyel tamas.k.lengyel@gmail.com napisał(a):

> On Tue, Jun 30, 2020 at 11:39 AM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>>
>> On 30/06/2020 13:33, Michał Leszczyński wrote:
>> > diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
>> > index b328a47ed8..0203029be9 100644
>> > --- 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)
>> > +#define  RTIT_CTL_CYCEN                      (_AC(1, ULL) <<  1)
>>
>> In addition to what Jan has said, please can we be consistent with an
>> underscore (or not) before EN.  Preferably with, so these would become
>> TRACE_EN and CYC_EN.
>>
>> That said, there are a lot of bit definitions which aren't used at all.
>> IMO, it would be better to introduce defines when you use them.
> 
> In the past I found it very valuable when this type of plumbing was
> already present in Xen instead of me having to go into the SDM to digg
> out the magic numbers. So while some of the bits might not be used
> right now I also don't see any downside in having them, just in case.
> 
> Tamas


+1 for keeping the unused #defines, this is a helpful piece of knowledge
which speeds up further patch development. It doesn't affect the compilation
nor runtime time and it doesn't occupy too much space in the code so I would
opt for keep it.

I will rebase this series onto latest master within patch v5. The remaining
patches in this series are not affected and still could be reviewed,
so I will wait a few days before posting the new version.


Best regards,
Michał Leszczyński
CERT Polska
Andrew Cooper July 1, 2020, 5:52 p.m. UTC | #5
On 30/06/2020 13:33, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@cert.pl>
>
> Define constants related to Intel Processor Trace features.
>
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>

Acked-by: Andrew Cooper <andrew.cooper3@ctirix.com>

I wanted to have a play with the series, and have ended up having to do
the rebase anyway.

As we're in code freeze for 4.14, I've started x86-next in its usual
location
(https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next)
and will commit this (and any other accumulated patches) once 4.15 opens.

~Andrew

Patch
diff mbox series

diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index b328a47ed8..0203029be9 100644
--- 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)
+#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                   (_AC(0x0F, ULL) <<  14)
+#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))))
+
+#define MSR_RTIT_STATUS                     0x00000571
+#define  RTIT_STATUS_FILTER_EN               (_AC(1, ULL) <<  0)
+#define  RTIT_STATUS_CONTEXT_EN              (_AC(1, ULL) <<  1)
+#define  RTIT_STATUS_TRIGGER_EN              (_AC(1, ULL) <<  2)
+#define  RTIT_STATUS_ERROR                   (_AC(1, ULL) <<  4)
+#define  RTIT_STATUS_STOPPED                 (_AC(1, ULL) <<  5)
+#define  RTIT_STATUS_BYTECNT                 (_AC(0x1FFFF, ULL) <<  32)
+
+#define MSR_RTIT_CR3_MATCH                  0x00000572
+#define MSR_RTIT_ADDR_A(n)                  (0x00000580 + (n) * 2)
+#define MSR_RTIT_ADDR_B(n)                  (0x00000581 + (n) * 2)
+
 #define MSR_U_CET                           0x000006a0
 #define MSR_S_CET                           0x000006a2
 #define  CET_SHSTK_EN                       (_AC(1, ULL) <<  0)