On 08/22/18 09:13, Wang, Jian J wrote:
> Hi Laszlo,
>
> Regards,
> Jian
>
> From: Laszlo Ersek [mailto:[email protected]]
> Sent: Tuesday, August 21, 2018 10:59 PM
> To: Wang, Jian J <[email protected]>; [email protected]
> Cc: Ni, Ruiyu <[email protected]>; Dong, Eric <[email protected]>
> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
