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

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

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?


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


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


> 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