Re: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.

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

Hi Eric,

I recommend that PROC_TRACE_TOPA_ENTRY be added to
UefiCpuPkg\Include\Register\ArchitecturalMsr.h after
MSR_IA32_RTIT_CTL_REGISTER that contains the TOPA 
enable bit.

How is the value of MAX_TOPA_ENTRY_COUNT determined?
If that value of 2 is from the SDM, we may want to 
add this define to ArchitecturalMsr.h too.

Mike

> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, August 16, 2017 11:36 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data
> structure when change MSR value.
> 
> When update MSR values, current code use BITxx to modify the
> MSR values. Enhance
> the code to use corresponding MSR's data structures to make it
> more user friendly.
> 
> 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>
> ---
>  .../Library/CpuCommonFeaturesLib/ProcTrace.c       | 112
> +++++++++++++++------
>  1 file changed, 84 insertions(+), 28 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> index 40e6321..fa458ab 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> @@ -69,8 +69,50 @@ typedef struct  {
>    PROC_TRACE_PROCESSOR_DATA   *ProcessorData;
>  } PROC_TRACE_DATA;
> 
> +typedef union {
> +  ///
> +  /// Individual bit fields
> +  ///
> +  struct {
> +    ///
> +    /// [Bit 0] END. See Section 35.2.6.2, "Table of Physical
> Addresses (ToPA)".
> +    ///
> +    UINT64  END:1;
> +    UINT64  Reserved1:1;
> +    ///
> +    /// [Bit 2] INT. See Section 35.2.6.2, "Table of Physical
> Addresses (ToPA)".
> +    ///
> +    UINT64  INT:1;
> +    UINT64  Reserved2:1;
> +    ///
> +    /// [Bit 4] STOP. See Section 35.2.6.2, "Table of
> Physical Addresses (ToPA)".
> +    ///
> +    UINT64  STOP:1;
> +    UINT64  Reserved3:1;
> +    ///
> +    /// [Bit 6:9] Indicates the size of the associated output
> region. See Section
> +    /// 35.2.6.2, "Table of Physical Addresses (ToPA)".
> +    ///
> +    UINT64  Size:4;
> +    UINT64  Reserved4:2;
> +    ///
> +    /// [Bit 12:63] Output Region Base Physical Address.
> +    /// 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)".
> +    ///
> +    UINT64  Address:52;
> +  } Bits;
> +  ///
> +  /// All bit fields as a 64-bit value
> +  ///
> +  UINT64  Uint64;
> +} PROC_TRACE_TOPA_ENTRY;
> +
>  typedef struct {
> -  UINT64   TopaEntry[MAX_TOPA_ENTRY_COUNT];
> +  PROC_TRACE_TOPA_ENTRY    TopaEntry[MAX_TOPA_ENTRY_COUNT];
>  } PROC_TRACE_TOPA_TABLE;
> 
>  /**
> @@ -186,7 +228,6 @@ ProcTraceInitialize (
>    IN BOOLEAN                           State
>    )
>  {
> -  UINT64                               MsrValue;
>    UINT32                               MemRegionSize;
>    UINTN                                Pages;
>    UINTN                                Alignment;
> @@ -199,6 +240,11 @@ ProcTraceInitialize (
>    PROC_TRACE_TOPA_TABLE                *TopaTable;
>    PROC_TRACE_DATA                      *ProcTraceData;
>    BOOLEAN                              FirstIn;
> +  MSR_IA32_RTIT_CTL_REGISTER           CtrlReg;
> +  MSR_IA32_RTIT_STATUS_REGISTER        StatusReg;
> +  MSR_IA32_RTIT_OUTPUT_BASE_REGISTER   OutputBaseReg;
> +  MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER  OutputMaskPtrsReg;
> +  PROC_TRACE_TOPA_ENTRY                *TopaEntryPtr;
> 
>    ProcTraceData = (PROC_TRACE_DATA *) ConfigData;
> 
> @@ -221,29 +267,28 @@ ProcTraceInitialize (
>    //
>    // Clear MSR_IA32_RTIT_CTL[0] and IA32_RTIT_STS only if
> MSR_IA32_RTIT_CTL[0]==1b
>    //
> -  MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> -  if ((MsrValue & BIT0) != 0) {
> +  CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> +  if (CtrlReg.Bits.TraceEn != 0) {
>      ///
>      /// Clear bit 0 in MSR IA32_RTIT_CTL (570)
>      ///
> -    MsrValue &= (UINT64) ~BIT0;
> +    CtrlReg.Bits.TraceEn = 0;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_CTL,
> -      MsrValue
> +      CtrlReg.Uint64
>        );
> 
>      ///
>      /// Clear MSR IA32_RTIT_STS (571h) to all zeros
>      ///
> -    MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_STATUS);
> -    MsrValue &= 0x0;
> +    StatusReg.Uint64 = 0x0;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_STATUS,
> -      MsrValue
> +      StatusReg.Uint64
>        );
>    }
> 
> @@ -309,35 +354,35 @@ ProcTraceInitialize (
>      //
>      // Clear MSR IA32_RTIT_CTL (0x570) ToPA (Bit 8)
>      //
> -    MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> -    MsrValue &= (UINT64) ~BIT8;
> +    CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> +    CtrlReg.Bits.ToPA = 0;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_CTL,
> -      MsrValue
> +      CtrlReg.Uint64
>        );
> 
>      //
>      // Program MSR IA32_RTIT_OUTPUT_BASE (0x560) bits[47:12]
> with the allocated Memory Region
>      //
> -    MsrValue = (UINT64) MemRegionBaseAddr;
> +    OutputBaseReg.Uint64 = (UINT64) MemRegionBaseAddr;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_OUTPUT_BASE,
> -      MsrValue
> +      OutputBaseReg.Uint64
>        );
> 
>      //
>      // Program the Mask bits for the Memory Region to MSR
> IA32_RTIT_OUTPUT_MASK_PTRS (561h)
>      //
> -    MsrValue = (UINT64) MemRegionSize - 1;
> +    OutputMaskPtrsReg.Uint64 = (UINT64) MemRegionSize - 1;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_OUTPUT_MASK_PTRS,
> -      MsrValue
> +      OutputMaskPtrsReg.Uint64
>        );
> 
>    }
> @@ -408,55 +453,66 @@ ProcTraceInitialize (
>      }
> 
>      TopaTable = (PROC_TRACE_TOPA_TABLE *) TopaTableBaseAddr;
> -    TopaTable->TopaEntry[0] = (UINT64) (MemRegionBaseAddr |
> ((ProcTraceData->ProcTraceMemSize) << 6)) & ~BIT0;
> -    TopaTable->TopaEntry[1] = (UINT64) TopaTableBaseAddr |
> BIT0;
> +    TopaEntryPtr = &TopaTable->TopaEntry[0];
> +    TopaEntryPtr->Bits.Address = MemRegionBaseAddr;
> +    TopaEntryPtr->Bits.Size = ProcTraceData-
> >ProcTraceMemSize;
> +    TopaEntryPtr->Bits.END = 0;
> +
> +    TopaEntryPtr = &TopaTable->TopaEntry[1];
> +    TopaEntryPtr->Bits.Address = TopaTableBaseAddr;
> +    TopaEntryPtr->Bits.END = 1;
> 
>      //
>      // Program the MSR IA32_RTIT_OUTPUT_BASE (0x560)
> bits[47:12] with ToPA base
>      //
> -    MsrValue = (UINT64) TopaTableBaseAddr;
> +    OutputBaseReg.Uint64 = (UINT64) TopaTableBaseAddr;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_OUTPUT_BASE,
> -      MsrValue
> +      OutputBaseReg.Uint64
>        );
> 
>      //
>      // Set the MSR IA32_RTIT_OUTPUT_MASK (0x561) bits[63:7]
> to 0
>      //
> +    OutputMaskPtrsReg.Uint64 = (UINT64) 0x7F;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_OUTPUT_MASK_PTRS,
> -      0x7F
> +      OutputMaskPtrsReg.Uint64
>        );
>      //
>      // Enable ToPA output scheme by enabling MSR
> IA32_RTIT_CTL (0x570) ToPA (Bit 8)
>      //
> -    MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> -    MsrValue |= BIT8;
> +    CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> +    CtrlReg.Bits.ToPA = 1;
>      CPU_REGISTER_TABLE_WRITE64 (
>        ProcessorNumber,
>        Msr,
>        MSR_IA32_RTIT_CTL,
> -      MsrValue
> +      CtrlReg.Uint64
>        );
>    }
> 
>    ///
>    /// Enable the Processor Trace feature from MSR
> IA32_RTIT_CTL (570h)
>    ///
> -  MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> -  MsrValue |= (UINT64) BIT0 + BIT2 + BIT3 + BIT13;
> +  CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
> +  CtrlReg.Bits.OS = 1;
> +  CtrlReg.Bits.User = 1;
> +  CtrlReg.Bits.BranchEn = 1;
>    if (!State) {
> -    MsrValue &= (UINT64) ~BIT0;
> +    CtrlReg.Bits.TraceEn = 0;
> +  } else {
> +    CtrlReg.Bits.TraceEn = 1;
>    }
>    CPU_REGISTER_TABLE_WRITE64 (
>      ProcessorNumber,
>      Msr,
>      MSR_IA32_RTIT_CTL,
> -    MsrValue
> +    CtrlReg.Uint64
>      );
> 
>    return RETURN_SUCCESS;
> --
> 2.7.0.windows.1