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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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:
> > thanks laszlo, for the detail review
> >
> > I have not gone through every line of code in detail. Some comment in
> > general.
> >
> > To answer Laszlo's question on Mmio.
> > No, the Mmio cannot be used as communication buffer. But the smm must
> > setup page table for it because the smm device driver may need access
> > it.
> 
> Makes sense. In that case, the comment on the "access-out disabled" case
> should mention MMIO.
> 
> 
> > I am not sure the difference between Mmio and runtime Mmio. Runtime is
> > only useful concept for Uefi, but not for Smm.
> >
> > Back to this patch itself.
> > I feel *guilty* when I see a new pcd introduced to control the code
> > flow.
> > That means the possible number of code path is doubled. Sigh...
> >
> > The first question is: what unit test has been run?
> > Does the unit test cover all possible true/false combination with
> > other PCD?
> > We have a big table to describe all legal or illegal combination for
> > the pcd to support memory protection. I think we have to update that
> > table if we decide to add the new one.
> >
> > Maybe we can think of what is the supported  case. Maybe we use an
> > *enum* to indicate the supported cases to reduce the number.
> > If the number is 4, no difference.
> > If the number is 3, I recommend to use enum type instead of a new
> > Boolean.
> > If the number is 2, I don't recommend we add new Boolean at all. We
> > can reinterpret the existing one.
> >
> >
> >
> > Below is my thought, please correct if I am wrong.
> > 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.

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


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



> 
> > As such I recommend we only support #2 and #3.
> >
> > Again, if the naming is confusing, I agree we should clarify or even
> > rename.
> > What I am trying to achieve is to limit the number of supported
> > combination to reduce the effort of validation and maintenance.
> 
> I agree. IMO:
> 
> - we should represent separate concepts separately,
> - exclude those combinations that make no sense
>   (with an ASSERT + appropriate comment)
> - exclude those combinations that make sense but are
>   unimportant or too complex to support
>   (with a different ASSERT + proper comment).
> 
> Thanks
> Laszlo
> 
>