Re: [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP

["Ni, Ray" <ray.ni@intel.com>]

> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle   ##
> SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset                ##
> SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme      ##
> SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize           ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle       ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset                    ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme          ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize               ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly     ##

1. PcdCpuProcTraceBspOnly? This is to use the same prefix as other ProcTrace PCDs.


> 
> +  IsBsp           = (CpuInfo->ProcessorInfo.StatusFlag & BIT0) ? TRUE : FALSE;
2. Can you use PROCESSOR_AS_BSP_BIT instead of BIT0?


> +  EnableOnBspOnly = (PcdGetBool (PcdEnableProcessorTraceOnBspOnly)) ?
> TRUE : FALSE;

3. why not directly use PcdGetBool (PcdEnableProcessorTraceOnBspOnly) in below if-check?

> +
> +  if (EnableOnBspOnly && (IsBsp == FALSE)) {

4. Change "IsBsp == FALSE" to "!IsBsp".




> +    if (EnableOnBspOnly) {
>        AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages,
> Alignment);
>        if (AlignedAddress == 0) {
> -        DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated only for %d
> threads\n", ProcTraceData->AllocatedThreads));
> -        if (Index == 0) {
> -          //
> -          // Could not allocate for BSP even
> -          //
> -          FreePool ((VOID *)ThreadMemRegionTable);
> -          ThreadMemRegionTable = NULL;
> -          return RETURN_OUT_OF_RESOURCES;
> +        //
> +        // Could not allocate for BSP even
> +        //
> +        DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, failed to allocate
> buffer for BSP\n"));
> +        return RETURN_OUT_OF_RESOURCES;
> +      }
> +
> +      DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for BSP only.\n"));

5. AlignedAddress is the proc trace buffer for BSP. But it seems the value is not used in
   later MSR_IA32_RTIT_OUTPUT_BASE programming.
    Similar comments to the  Topa code path.