Re: [Patch v2 1/2] UefiCpuPkg/ArchitecturalMsr.h: Add RTIT TOPA table entry definition.

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Hi Eric,

I see that the legal values for the Size field of 
RTIT_TOPA_TABLE_ENTRY are documented in the SDM.

I think we should add the enum for PROC_TRACE_MEM_SIZE
into ArchitecturalMsr.h next to RTIT_TOPA_TABLE_ENTRY.
Maybe rename it to RTIT_TOPA_MEMORY_SIZE with enum names
that start with "RtitTopaMemorySize".

The Copyright date in ArchitecturalMsr.h also needs to 
be updated.

The #define for MAX_TOPA_ENTRY_COUNT need a comment block
that describes what it is and why it has a value of 2.  I see
2 entries are needed because the list of entries must be terminated
by an entry with the END bit set to 1, so 2 entries are required
to use a single valid entry.

ProcTrace.c: Please update programming of MSR_IA32_RTIT_OUTPUT_BASE
to use the named bit fields from the structure.

ProcTrace.c: Please update programming of MSR_IA32_RTIT_OUTPUT_MASK_PTRS
to use the named bit fields from the structure.

ProcTrace.c: I am confused by the programming
MSR_IA32_RTIT_OUTPUT_MASK_PTRS to 0x7f.  That sets a Reserved
field to all 1s.

ProcTrace.c: I see a mix of setting TopaEntryPtr using both
bit fields and the Uint64 field.  Can we change to use only
the bit fields.  Do we really need to set the address field 
in entry #1?  Isn't setting END to 1 enough?

Thanks,

Mike

> -----Original Message-----
> From: Dong, Eric
> Sent: Thursday, August 17, 2017 8:29 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: [Patch v2 1/2] UefiCpuPkg/ArchitecturalMsr.h: Add RTIT
> TOPA table entry definition.
> 
> Add RTIT TOPA table entry definition to architecturalMsr.h file.
> 
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Include/Register/ArchitecturalMsr.h | 55
> ++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/Register/ArchitecturalMsr.h
> b/UefiCpuPkg/Include/Register/ArchitecturalMsr.h
> index 4f9c103..40c4383 100644
> --- a/UefiCpuPkg/Include/Register/ArchitecturalMsr.h
> +++ b/UefiCpuPkg/Include/Register/ArchitecturalMsr.h
> @@ -4534,6 +4534,61 @@ typedef union {
>    UINT64  Uint64;
>  } MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER;
> 
> +/**
> +  Format of ToPA table entries.
> +**/
> +typedef union {
> +  ///
> +  /// Individual bit fields
> +  ///
> +  struct {
> +    ///
> +    /// [Bit 0] END. See Section 35.2.6.2, "Table of Physical
> Addresses (ToPA)".
> +    ///
> +    UINT32  END:1;
> +    UINT32  Reserved1:1;
> +    ///
> +    /// [Bit 2] INT. See Section 35.2.6.2, "Table of Physical
> Addresses (ToPA)".
> +    ///
> +    UINT32  INT:1;
> +    UINT32  Reserved2:1;
> +    ///
> +    /// [Bit 4] STOP. See Section 35.2.6.2, "Table of Physical
> Addresses (ToPA)".
> +    ///
> +    UINT32  STOP:1;
> +    UINT32  Reserved3:1;
> +    ///
> +    /// [Bit 6:9] Indicates the size of the associated output
> region. See Section
> +    /// 35.2.6.2, "Table of Physical Addresses (ToPA)".
> +    ///
> +    UINT32  Size:4;
> +    UINT32  Reserved4:2;
> +    ///
> +    /// [Bit 12:31] Output Region Base Physical Address low part.
> +    /// [Bit 12:31] Output Region Base Physical Address [12:63]
> value to match.
> +    /// ATTENTION: The size of the address field is determined by
> the processor's
> +    /// physical-address width (MAXPHYADDR) in bits, as reported
> in
> +    /// CPUID.80000008H:EAX[7:0]. the above part of address
> reserved.
> +    /// True address field is [12:MAXPHYADDR-1], [MAXPHYADDR:63]
> is reserved part.
> +    /// Detail see Section 35.2.6.2, "Table of Physical Addresses
> (ToPA)".
> +    ///
> +    UINT32  Base:20;
> +    ///
> +    /// [Bit 32:63] Output Region Base Physical Address high
> part.
> +    /// [Bit 32:63] Output Region Base Physical Address [12:63]
> value to match.
> +    /// ATTENTION: The size of the address field is determined by
> the processor's
> +    /// physical-address width (MAXPHYADDR) in bits, as reported
> in
> +    /// CPUID.80000008H:EAX[7:0]. the above part of address
> reserved.
> +    /// True address field is [12:MAXPHYADDR-1], [MAXPHYADDR:63]
> is reserved part.
> +    /// Detail see Section 35.2.6.2, "Table of Physical Addresses
> (ToPA)".
> +    ///
> +    UINT32  BaseHi:32;
> +  } Bits;
> +  ///
> +  /// All bit fields as a 64-bit value
> +  ///
> +  UINT64  Uint64;
> +} RTIT_TOPA_TABLE_ENTRY;
> 
>  /**
>    Trace Control Register (R/W). If (CPUID.(EAX=07H,
> ECX=0):EBX[25] = 1).
> --
> 2.7.0.windows.1