Laszlo,
> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, February 28, 2019 10:07 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH 0/2] Fix SMM driver hang at accessing memory > beyond 4G > > 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. > Correct. > > (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. > > Agree. I'll merge them. > (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). > > Very good explanation and abstraction. Agree. > (4) I also suggest adding: > > Fixes: dcc026217fdc363f55c217039fc43d344f69fed6 > > near the end of the commit message. > > > With (2) through (4) addressed: > > Acked-by: Laszlo Ersek <ler...@redhat.com> > It'll be added. Thanks for the great feedback. Jian > Thanks > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
