[edk2-devel] [Patch V2 1/3] UefiCpuPkg: Add Unit tests for DxeCpuExceptionHandlerLib

[Ni, Ray]

the patch looks good!
5 minor comments in below.

> since [StackBase, StackBase + SIZE_4KB] is guarded in page
1. can you say specifically "... is marked as not present in page table" instead of "guarded"?

> +#pragma pack (1)
> +
> +typedef union {
> +  struct {
> +    UINT32    LimitLow    : 16;
> +    UINT32    BaseLow     : 16;
> +    UINT32    BaseMid     : 8;
> +    UINT32    Type        : 4;
> +    UINT32    System      : 1;
> +    UINT32    Dpl         : 2;
> +    UINT32    Present     : 1;
> +    UINT32    LimitHigh   : 4;
> +    UINT32    Software    : 1;
> +    UINT32    Reserved    : 1;
> +    UINT32    DefaultSize : 1;
> +    UINT32    Granularity : 1;
> +    UINT32    BaseHigh    : 8;
> +  } Bits;
> +  UINT64    Uint64;
> +} IA32_GDT;

2. can you reuse IA32_SEGMENT_DESCRIPTOR definition from BaseLib.h?

> +
> +typedef struct {
> +  UINT32    InitialApicId;
> +  UINT32    ApicId;
> +  UINT32    Health;
> +  UINT64    ApTopOfStack;
> +} CPU_INFO_IN_HOB;

3. This is an internal data structure used by MpInitLib. Do you still need it?

> +typedef struct {
> +  UINT64    Rdi;
> +  UINT64    Rsi;
> +  UINT64    Rbx;
> +  UINT64    Rdx;
> +  UINT64    Rcx;
> +  UINT64    Rax;
> +  UINT64    R8Register;
> +  UINT64    R9Register;
> +  UINT64    R10Register;
> +  UINT64    R11Register;
> +  UINT64    R12Register;
> +  UINT64    R13Register;
> +  UINT64    R14Register;
> +  UINT64    R15Register;

4. Can we just use "R8/R9" as the register name? If needed, you can change the ECC exception file to fix the ECC failure.



> +
> +  Cr0.UintN = AsmReadCr0 ();
> +  if (Cr0.Bits.PG == 0) {
> +    return;
5. FoundPFAddress is not set to FALSE when returning.
    How about let the function return a Boolean flag so the caller code can be as below?
    if (FindPFAddressInPageTable (&PFAddress)) {
       ...
    }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95190): https://edk2.groups.io/g/devel/message/95190
Mute This Topic: https://groups.io/mt/94319170/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-