Thanks Marvin, I will fix according the comments. For 2), I don’t see assert only code in my patch. The only allocation in this patch is below, which has already been handled by if check.

 

  if (mSmmInitialized == NULL) {

    mSmmInitialized = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * mMaxNumberOfCpus);

  }

 

  ASSERT (mSmmInitialized != NULL);

  if (mSmmInitialized == NULL) {

    return;

  }

 

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin H?user
Sent: Friday, February 10, 2023 6:00 PM
To: Wu; Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

 

Hi Jiaxin,

1) mSmmInitialized *must* be volatile. Your current code may cause anything, from skipping waiting entirely (the loop is optimized away as the compiler considers it free from side effects) to stalling (the memory access is optimized away as the compiler considers it locally-immutable).

2) ASSERTs on memory allocation failures are one of the most terrible edk2 "paradigms".

3) Comparisons against boolean constants are not allowed by the code style spec.

Best regards,
Marvin