Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging

["Zhiguang Liu" <zhiguang.liu@intel.com>]

Hi Gerd,

Let's check the code in InitPaging.
If 5LevelPaging is disabled, Pml5 points to a local variable. Pml5[1] shouldn't be used.

     UINT64    Pml5Entry;
     UINT64    *Pml5;
     if (!Enable5LevelPaging) {
        Pml5Entry = (UINTN)mSmmProfileCr3 | IA32_PG_P;
        Pml5      = &Pml5Entry;

However, if NumberOfPml5Entries is larger than 1, below code will access Pml5[1], which may cause unexpected future code flow.

    for (Pml5Index = 0; Pml5Index < NumberOfPml5Entries; Pml5Index++) {
        if ((Pml5[Pml5Index] & IA32_PG_P) == 0) {

Could this can answer your question? Please let me know if you still have concern.

And for the CpuPageTableLib, I think the API don't provide the interface to split 2MB-page page table into 4KB-page, which is the function wants to do.

Thanks
Zhiguang


> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Wednesday, January 18, 2023 4:54 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when
> InitPaging
> 
> On Wed, Jan 18, 2023 at 01:13:43AM +0000, Zhiguang Liu wrote:
> > Thanks all for reviewing, and I will send a new version to address the comment.
> >
> > As for Gerd's question, let me explain.
> > Let's see one example, that the CPU has SizeOfMemorySpace >48, but the CPU
> doesn't enable 5 level paging.
> > The purpose of the current function InitPaging is to modify existing
> > page table. To use the same logic to handle both 5 level and 4 level
> > paging, for 4 level paging, the logic will create a false 5 level
> > paging entry to treat it like a 5 level page table.
> 
> Yes.  Same for 3-level paging btw.  There are always page tables for 5 levels, but
> the higher levels might be unused.
> 
> > This way, the
> > number of 5 level paging should always be one. If we use
> > SizeOfMemorySpace to calculate the 5 level paging entry count, we will
> > get number more than one.  However, as I just mentioned, we only
> > create one false 5 level paging entry, system may hang when we try to
> > access the second 5 level paging entry.
> 
> If 5-level paging is turned off the CPU should not see what you are doing with
> the page tables for the second (and higher) 5-level entry.
> 
> So, limiting the number of 5-level entries does make sense.  Higher entries are
> not used, so it's pointless work.
> 
> But that doesn't answer the question:  Why does that fix the system hanging?  I
> just can't see a reason for that when looking through the InitPaging code.  I
> suspect this might hide a bug somewhere else.
> 
> Related:  We got UefiCpuPkg/Library/CpuPageTableLib last year, can this be
> used instead?
> 
> take care,
>   Gerd