Re: [Patch 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove redundant definition.

["Dong, Eric" <eric.dong@intel.com>]

Mike,

The new check for ProcTraceOutputScheme is for the functionality which is missed before. the user selection and hardware capability may not consistent. So I add this new check.

I agree to keep the validate check. Please check the new patch.

Thanks,
Eric
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D
Sent: Thursday, August 24, 2017 7:03 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: [edk2] [Patch 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove redundant definition.

Eric,

Comment below.

Mike

> -----Original Message-----
> From: Dong, Eric
> Sent: Tuesday, August 22, 2017 7:31 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu 
> <ruiyu.ni@intel.com>
> Subject: [Patch 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove redundant 
> definition.
> 
> The EnumProcTraceMemDisable/OutputSchemeInvalid are redundant 
> definitions. These definitions can be handled by other code, so remove 
> them.
> 
> 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/Library/CpuCommonFeaturesLib/ProcTrace.c | 14 ++++----
> ------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> index a90dd4e..6524882 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> @@ -35,8 +35,7 @@ typedef enum {
>    Enum16M,
>    Enum32M,
>    Enum64M,
> -  Enum128M,
> -  EnumProcTraceMemDisable
> +  Enum128M
>  } PROC_TRACE_MEM_SIZE;
> 
>  ///
> @@ -44,8 +43,7 @@ typedef enum {
>  ///
>  typedef enum {
>    OutputSchemeSingleRange = 0,
> -  OutputSchemeToPA,
> -  OutputSchemeInvalid
> +  OutputSchemeToPA
>  } PROC_TRACE_OUTPUT_SCHEME;
> 
>  typedef struct  {
> @@ -134,10 +132,6 @@ ProcTraceSupport (
>    // Check if ProcTraceMemorySize option is enabled (0xFF means 
> disable by user)
>    //
>    ProcTraceData = (PROC_TRACE_DATA *) ConfigData;
> -  if ((ProcTraceData->ProcTraceMemSize >=
> EnumProcTraceMemDisable) ||
> -      (ProcTraceData->ProcTraceOutputScheme >=
> OutputSchemeInvalid)) {
> -    return FALSE;
> -  }

I see the ProcTraceOutputScheme values are checked below.
Do we need to keep the check for a valid ProcTraceMemSize value?

> 
>    //
>    // Check if Processor Trace is supported @@ -151,8 +145,8 @@ 
> ProcTraceSupport (
>    AsmCpuidEx (CPUID_INTEL_PROCESSOR_TRACE, 
> CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF, NULL, NULL, &Ecx.Uint32, NULL);
>    ProcTraceData->ProcessorData[ProcessorNumber].TopaSupported =
> (BOOLEAN) (Ecx.Bits.RTIT == 1);
>    ProcTraceData-
> >ProcessorData[ProcessorNumber].SingleRangeSupported = (BOOLEAN)
> (Ecx.Bits.SingleRangeOutput == 1);
> -  if (ProcTraceData->ProcessorData[ProcessorNumber].TopaSupported
> ||
> -      ProcTraceData-
> >ProcessorData[ProcessorNumber].SingleRangeSupported) {
> +  if ((ProcTraceData-
> >ProcessorData[ProcessorNumber].TopaSupported && (ProcTraceData- 
> >ProcTraceOutputScheme == OutputSchemeToPA)) ||
> +      (ProcTraceData-
> >ProcessorData[ProcessorNumber].SingleRangeSupported &&
> (ProcTraceData->ProcTraceOutputScheme ==
> OutputSchemeSingleRange))) {
>      return TRUE;
>    }
> 
> --
> 2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel