From: "Ni, Ray" <ray.ni@intel.com>
To: "Tan, Dun" <dun.tan@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Dong, Eric" <eric.dong@intel.com>,
"Kumar, Rahul R" <rahul.r.kumar@intel.com>
Subject: Re: [Patch V2 1/3] UefiCpuPkg: Add Unit tests for DxeCpuExceptionHandlerLib
Date: Fri, 14 Oct 2022 05:48:35 +0000 [thread overview]
Message-ID: <MWHPR11MB1631910B15DAD52A1ED81FE08C249@MWHPR11MB1631.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20221014015400.440-2-dun.tan@intel.com>
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)) {
...
}
next prev parent reply other threads:[~2022-10-14 5:48 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-14 1:53 [Patch V2 0/3] Add Unit tests for Pei/DxeCpuExceptionHandlerLib duntan
2022-10-14 1:53 ` [Patch V2 1/3] UefiCpuPkg: Add Unit tests for DxeCpuExceptionHandlerLib duntan
2022-10-14 5:48 ` Ni, Ray [this message]
2022-10-14 6:17 ` duntan
2022-10-14 1:53 ` [Patch V2 2/3] UefiCpuPkg: Add Unit tests for PeiCpuExceptionHandlerLib duntan
2022-10-14 5:54 ` Ni, Ray
2022-10-14 6:16 ` duntan
2022-10-14 1:54 ` [Patch V2 3/3] UefiCpuPkg: Add Pei/DxeCpuExceptionHandlerLibUnitTest in dsc duntan
2022-10-14 5:54 ` Ni, Ray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR11MB1631910B15DAD52A1ED81FE08C249@MWHPR11MB1631.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox