public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Dong, Eric" <eric.dong@intel.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>
Subject: Re: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.
Date: Fri, 18 Aug 2017 03:31:18 +0000	[thread overview]
Message-ID: <ED077930C258884BBCB450DB737E66224A9C1EEC@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5A7D81E5F@ORSMSX113.amr.corp.intel.com>

Hi Mike,

I have updated the patch to follow your suggest. Please help to review the new patches.

BTW, why we can't use UINT64 bit fields? Does it have potential issue in 32 bits system?

Thanks,
Eric
From: Kinney, Michael D
Sent: Friday, August 18, 2017 1:33 AM
To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.

Eric,

You should not us UINT64 for bitfields.  Please use UINT32 instead, and make sure bitfields do not cross 32-bit boundaries. Other than that, adding this structure to ArchitectureMsrs.h with the comments you have looks correct to me.

I agree that MAX_TOPA_ENTRY_COUNT should remain in the CpuCommonFeaturesLib implementation.

Thanks,

Mike

From: Dong, Eric
Sent: Thursday, August 17, 2017 1:27 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.


Mike,



The PROC_TRACE_TOPA_ENTRY definition has a little difference with the spec definition, as I have raised in the comments  for "Address" field.

    ///

    /// [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;



The spec description for this entry like below:

[cid:image001.png@01D31815.819100E0]



For my use case in this file, I think this definition is ok and make the code more user friendly, so I defined it in my local file. But I'm not sure whether other cases can use this definition if I put this definition to ArchitecturalMsr.h.





For the MAX_TOPA_ENTRY_COUNT macro, it's implement choice. So it can't move to ArchitecturalMsr.h.



Thanks,

Eric



-----Original Message-----
From: Kinney, Michael D
Sent: Thursday, August 17, 2017 3:53 PM
To: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.



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<mailto:edk2-devel@lists.01.org>

> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Ni, Ruiyu

> <ruiyu.ni@intel.com<mailto: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<mailto:michael.d.kinney@intel.com>>

> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto: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




  reply	other threads:[~2017-08-18  3:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-17  6:35 [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value Eric Dong
2017-08-17  7:53 ` Kinney, Michael D
2017-08-17  8:26   ` Dong, Eric
2017-08-17 17:32     ` Kinney, Michael D
2017-08-18  3:31       ` Dong, Eric [this message]
2017-08-18  7:38         ` Kinney, Michael D

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ED077930C258884BBCB450DB737E66224A9C1EEC@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox