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

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

By the way, I know that ProcTraceInitialize() runs on BSP so it's safe to access the PCD database.
But to be consistent with the old logic, can you please cache the PCD value in ProcTraceGetConfigData()?
It has slight benefit that the PCD is only accessed once not multiple times depending on cpu count.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, April 25, 2023 2:03 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Chen,
> Xiao X <xiao.x.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Update code to support
> enable ProcTrace only on BSP
> 
> > -  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.
> 
> 
> 
> 
>