yes, I will close that Bugzilla. Thanks all for your feedback. Thanks, Eric
From: Yao, Jiewen <jiewen....@intel.com> Sent: Saturday, September 5, 2020 8:31 PM To: devel@edk2.groups.io; ler...@redhat.com; vanjeff_...@hotmail.com; Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com> Cc: Lou, Yun <yun....@intel.com> 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<mailto:devel@edk2.groups.io> > <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo Ersek > Sent: Friday, September 4, 2020 4:37 PM > To: Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; > devel@edk2.groups.io<mailto:devel@edk2.groups.io>; > vanjeff_...@hotmail.com<mailto:vanjeff_...@hotmail.com>; Dong, Eric > <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Ni, Ray > <ray...@intel.com<mailto:ray...@intel.com>> > Cc: Lou, Yun <yun....@intel.com<mailto:yun....@intel.com>> > 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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65064): https://edk2.groups.io/g/devel/message/65064 Mute This Topic: https://groups.io/mt/76625321/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
