Thanks and agree. Comment inlined.
thank you! Yao, Jiewen > 在 2019年8月3日，上午6:06，Laszlo Ersek <[email protected]> 写道： > >> On 08/02/19 04:46, Yao, Jiewen wrote: >> Thanks Laszlo. Comment below: >> >>> -----Original Message----- >>> From: [email protected] [mailto:[email protected]] On Behalf Of >>> Laszlo Ersek >>> Sent: Friday, August 2, 2019 10:05 AM >>> To: Yao, Jiewen <[email protected]>; [email protected] >>> Cc: Ni, Ray <[email protected]>; Dong, Eric <[email protected]>; Wang, >>> Jian J <[email protected]> >>> Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: >>> PcdCpuSmmAccessOut controls SMM access-out policy >>> >>> On 08/01/19 02:02, Yao, Jiewen wrote: > >>>> 1) StaticPaging=false, AccessOut=false: it seems invalid. If we >>>> don't support access out, why we need dynamic paging? >>> >>> The first patch that introduced "access-out" (as a use case) was commit >>> c60d36b4d1ee ("UefiCpuPkg/SmmCpu: Block access-out only when static >>> paging is used", 2018-11-08). Before that, there was no access-out. >>> >>> The first patch that distinguished "static page tables" from "dynamic >>> page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add >>> PcdCpuSmmStaticPageTable.", 2016-11-17) >>> >>> During those two years, we didn't support access-out, but allowed >>> platforms to choose between static/dynamic paging. Are we calling that >>> invalid in retrospect? >> >> [Jiewen] Right. Agree with you. >> Our original intension is to provide a secure configuration, as #2. >> But we notice it is not possible, due to some special RAS requirement. >> Then we have to move to #2, and make #1 invalid. > > OK. I don't mind the RAS requirement as long as option #3 remains viable. > >>>> 2) StaticPaging=false, AccessOut=true: it seems valid. We need access >>>> out, but we only want a small paging in the beginning. As such we use >>>> dynamic paging. This is to support Hotplug memory. >>> >>> This scenario doesn't look supportable on IA32. (Due to >>> StaticPaging=false.) I don't mean that it's impossible to implement, >>> just that the IA32 code today doesn't extend the page tables in response >>> to page faults. >> >> [Jiewen] You are right. >> >> Some background on dynamic paging. >> It is introduced to resolve X64 problem, where Intel CPU only support 2M >> paging, but large physical memory. We cannot support build static 2M paging >> inside of SMRAM for X64. >> So we introduce dynamic paging to support this configuration. >> >> For IA32, 5 pages are enough to cover 4G, in 2M paging. As such, I do not >> see the value to provide dynamic paging in Ia32. >> >> Or I could say: this #2 is only valid for X64. No need for IA32. > > That's fine, it's just whatever solution we choose, should not break > compilation or behavior on IA32. > > Also, preferably, if a PCD only makes sense on X64, then it should be > declared in the DEC file as such, so that IA32-only code referencing the > PCD not even compile. Otherwise confusion will result (people looking at > the IA32-only code will ask themselves, "what should I do about this PCD > here"?) [jiewen] make sense. Agree. > > >>>> 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure >>>> configuration. >>> >>> Agreed. >>> >>> >>>> 4) StaticPaging=true, AccessOut=true: it seems valid, but I do not see >>>> the value to support this. If we always allow access out, what is the >>>> value to set static paging. Or why we care the paging is static or >>>> dynamic? >>> >>> - the IA32 binary constructs all tables in advance, and it might want to >>> interact with the RAS controller in question. >>> >>> - the X64 binary wants to allocate the SMRAM for page tables in advance >>> during boot (and not in the page fault handler), and protect the SMM >>> page tables, but still interact with the RAS controller through normal >>> RAM. >>> >>> Apologies if I'm missing something obvious that invalidates the above >>> use cases. >> >> [Jiewen] >> Our first focus is for X64 when we design this feature, because RAS is only >> required by server. >> And all RAS servers use X64, I believe. >> >> I don't think we need consider this case. >> >> >> All in all, to make it easy, I would suggest to limit the configuration >> below: >> A) For IA32, we only consider #3. >> B) For X64, we consider #3 as default. Only back to #2 for RAS platform. > > I guess that works for me. How about the following: > > (1) rename PcdCpuSmmStaticPageTable to PcdCpuSmmRestrictedMemoryAccess; > default value TRUE. [jiewen] agree. I also recommend add some comments say this is the rename to old PcdCpuSmmStaticPageTable. > > (2) move the PCD from > > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > > to > > [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, > PcdsDynamicEx.X64] > > in the DEC file [jiewen] make sense. agree. > > > (3) update the documentation on the PCD precisely, in the DEC file -- > describe all aspects and consequences. Also describe that on IA32, the > PCD should be considered constant TRUE [jiewen] agree. > (4) OK, so this is another interesting issue: in the documentation of > "PcdCpuSmmProfileEnable", it is stated that "PcdCpuSmmStaticPageTable" > must be disabled for enabling "PcdCpuSmmProfileEnable". That is > *already* contradictory (without the feature being added), because IA32 > always uses statically built page tables, but the IA32 *code* actually > honors PcdCpuSmmProfileEnable. > > Therefore, the documentation on "PcdCpuSmmProfileEnable" has to be > updated first, separately, to reflect reality. This should be a separate > patch. And then it can be updated for PcdCpuSmmRestrictedMemoryAccess. [jiewen] agree. I think we can add more description on that. PcdCpuSmmProfileEnable should a debug feature only and never be used for production. While PcdCpuSmmRestrictedMemoryAccess is for production. > > > (5) rename mCpuSmmStaticPageTable in the code to > mCpuSmmRestrictedMemoryAccess. [jiewen] agree > (6) If some common code (built for both IA32 and X64) needs to depend on > this variable, then a helper function is need; returning constant TRUE > on IA32 and returning mCpuSmmRestrictedMemoryAccess on X64. [jiewen] agree. > > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44879): https://edk2.groups.io/g/devel/message/44879 Mute This Topic: https://groups.io/mt/32668874/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
