Re: [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi

[Laszlo Ersek <lersek@redhat.com>]

On 08/22/18 09:13, Wang, Jian J wrote:
> Hi Laszlo,
> 
> Regards,
> Jian
> 
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, August 21, 2018 10:59 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi

>> +/**
>> +  Find and display image base address and return image base and its entry point.
>> +
>> +  @param CurrentIp      Current instruction pointer.
>> +
>> +**/
>> +STATIC
>> +VOID
>> +DumpModuleImageInfo (
>> +  IN  UINTN              CurrentIp
>> +  )
>> +{
>> +  EFI_STATUS                           Status;
>> +  UINTN                                Pe32Data;
>> +  VOID                                 *PdbPointer;
>> +  VOID                                 *EntryPoint;
>> +
>> +  Pe32Data = PeCoffSearchImageBase (CurrentIp);
>> +  if (Pe32Data == 0) {
>> +    InternalPrintMessage ("!!!! Can't find image information. !!!!\n");
>> +  } else {
>> +    //
>> +    // Find Image Base entry point
>> +    //
>> +    Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>> +    if (EFI_ERROR (Status)) {
>> +      EntryPoint = NULL;
>> +    }
>> +    InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentIp);
>> +    PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>> +    if (PdbPointer != NULL) {
>> +      InternalPrintMessage ("%a", PdbPointer);
>> +    } else {
>> +      InternalPrintMessage ("(No PDB) " );
>> +    }
>> +    InternalPrintMessage (
>> +      " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>> +      (VOID *) Pe32Data,
>> +      EntryPoint
>> +      );
>> +  }
>> +}
> 
> (4) Why is this function copied here? From a quick check, it looks
> identical to DumpModuleImageInfo() in
> "UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c".
> 
> And, DumpModuleImageInfo() is an extern function in
> "UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h", and we
> are (likely) already linking against that library instance. As a result
> we'll have the same function twice in CpuDxe, once as STATIC, and
> another time as extern.
> 
> If this is a useful utility function, then it should be elevated to a
> public library API, and used from there. (I don't know where to add it,
> I just find this code duplication regrettable.)
> 
> [Jian] I reviewed the whole working model again. I think we don’t need to dump
> the image information here, because, if there’s an issue detected, the developers
> will disable the non-stop mode anyway in order to find the root cause. Then the
> image information can be dumped (normal mode). So we can just dump cpu
> information to tell the user that there’s an exception here. And dumping cpu
> information is already a public interface so we don’t need to duplicate any code
> here.

Oh cool. If we can eliminate the function altogether, that's best.

Thanks!
Laszlo