Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy

["Laszlo Ersek" <lersek@redhat.com>]

On 08/02/19 04:46, Yao, Jiewen wrote:
> Thanks Laszlo. Comment below:
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, August 2, 2019 10:05 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Wang,
>> Jian J <jian.j.wang@intel.com>
>> 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"?)


>>> 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.


(2) move the PCD from

[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]

to

[PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64,
PcdsDynamicEx.X64]

in the DEC file


(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


(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.


(5) rename mCpuSmmStaticPageTable in the code to
mCpuSmmRestrictedMemoryAccess.


(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.


Thanks
Laszlo