yes, I will close that Bugzilla. Thanks all for your feedback. Thanks, Eric From: Yao, Jiewen Sent: Saturday, September 5, 2020 8:31 PM To: devel@edk2.groups.io; lersek@redhat.com; vanjeff_919@hotmail.com; Dong, Eric ; Ni, Ray Cc: Lou, Yun Subject: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. Thank you Laszlo. Eric and I communicated with internal team. They understand that CPU is the only owner of the CPU state (CR3/DGT/IDT) and MP_SERVICE_PROTOCOL. Then we agree that the original test is invalid. As conclusion, we can withdraw this patch and close the Bugzilla with "not a defect". 😊 Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io > On Behalf Of Laszlo Ersek > Sent: Friday, September 4, 2020 4:37 PM > To: Yao, Jiewen >; devel@edk2.groups.io; > vanjeff_919@hotmail.com; Dong, Eric >; Ni, Ray > > > Cc: Lou, Yun > > Subject: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for > CR3/GDT/IDT. > > On 09/04/20 10:06, Yao, Jiewen wrote: > > When we say “validate”, we need get clear on what is the contract between > caller and callee, and what is the expectation of the validation. > > > > For example, in this case, the validation is limited to 4G or not 4G. Why? > > This function does not verify following, why? > > > > 1. if the GDT table is valid. > > 2. if the IDT table is valid > > 3. if the exception handler is valid > > 4. if the timer handler still works > > 5. if the page table is valid > > 6. if the page table is 1:1 mapping > > 7. if the page table still enforce the expected protection, such as RO, NX > > 8. …… > > Yes, very good point; it has crossed my mind before too. The currently > proposed checks verify the CR3 (but not the end of the root page > directory). They also don't try to walk the whole forest of page tables > and check every entry against 4GB (or, as you say, for 1:1 mapping). The > check covers the GDT and the IDT, but not the GDT and IDT entries > (segment granularity? direction of growth?) > > I'm OK with the proposed rudimentary checks because in my mind they are > supposed to catch only one idiosyncratic UEFI application. > > > If an application creates a crazy state, CpuDxe may pass the validation with > below 4G page table, but still fail to wake up APs. > > Yes, absolutely. > > > > > If it is the app’s responsibility to ensure the system in good state, the > validation is unnecessary. > > If it is the CpuDxe’s responsibility to ensure the system in good state, the > validation is insufficient. > > Agreed on both counts. > > I'm just under the impression that Eric has some internal use case that > doesn't let him fix the application -- or maybe there's no time left for > them for fixing the application. > > > > > I think we should wait. I am working with Eric to collect the requirement on > why test case is designed in this way. > > Sounds good, thanks! > > > > > DebugAgentDxe is a good case. It is for debug purpose. > > I believe there must be contract between CPU driver and DebugAgentDxe that > which status DebugAgentDxe may modify and which state DebugAgentDxe may > not. > > It is DebugAgentDxe’s responsibility to ensure the new system state is correct > and compatible with the CPU driver. > > Agreed 100% > > > With the contract, I don’t think CPU driver need validate the system state > changed by DebugAgentDxe. > > If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to > run. > > Agreed again. > > Thanks > Laszlo > > >