Re: [PATCH 0/2] Fix SMM driver hang at accessing memory beyond 4G

[Laszlo Ersek <lersek@redhat.com>]

Hi Jian,

On 02/28/19 11:10, Jian J Wang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1576
> Test:
> - Pass special test of accessing memory beyond 4G
> - Boot to OS with Qemu emulator platform (Fedora27, Ubuntu18.04,
>   Windows7, Windows10)
> 
> Jian J Wang (2):
>   UefiCpuPkg/CpuDxe: set TF bit in EFLAGS in C code
>   UefiCpuPkg/CpuDxe: remove code to set TF bit in EFLAGS
> 
>  UefiCpuPkg/CpuDxe/CpuPageTable.c                      | 11 ++++++++++-
>  .../Ia32/ExceptionHandlerAsm.nasm                     |  7 -------
>  .../X64/ExceptionHandlerAsm.nasm                      |  4 ----
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 

thanks for the good description in the BZ and the patches.

Also thanks for the good commit message on commit dcc026217fdc
("UefiCpuPkg/CpuDxe: implement non-stop mode for uefi", 2018-08-30),
which is a handy reminder about nonstop mode.


(1) My understanding is that, for the problem to arise, it is necessary
for a platform to set

  PcdCpuSmmStaticPageTable = FALSE

Because of that, I expect the code changes as well to be restricted to
such code paths (i.e. I expect the code changes to be invisible with
PcdCpuSmmStaticPageTable=TRUE). Therefore I'll skip regression testing
this series.


(2) Both commit messages say,

"the fix is to *move* the logic to C code part of page fault exception
handler"

(In fact, the commit messages are identical.)

Therefore I think the two patches should be squashed into one. It should
be one atomic code movement. For example, if someone bisects the git
history, and they check out the tree between the two patches, they will
have the TF bit logic in both places. That's probably not good.


(3) I think this is my most important comment for this series:

The subject lines of the patches do not state the *actual* change. The
actual change is not that we move the TF bit manipulation from assembly
code to C code. Instead, the change is that we make the TF setting
*conditional*. In other words, we restrict the setting of TF (--> the
single stepping, = the debug exception after re-executing the originally
faulting instruction) only if we *need* the debug exception, for
restoring the strict page attributes, after the faulting instruction is
allowed to pass.

In other words, we enable the "restore page attributes" logic (#DB
exception handler) only if there is a reason for that (= we are in
nonstop mode).

My apologies if the above paragraph is too verbose. I simply suggest
that the squashed patch have the following subject:

UefiCpuPkg: restore strict page attributes via #DB in nonstop mode only

(71 characters).


(4) I also suggest adding:

Fixes: dcc026217fdc363f55c217039fc43d344f69fed6

near the end of the commit message.


With (2) through (4) addressed:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo