Re: [PATCH V3 0/6] Enable SMM page level protection.

[Laszlo Ersek <lersek@redhat.com>]

On 11/11/16 13:59, Laszlo Ersek wrote:
> On 11/11/16 13:53, Yao, Jiewen wrote:
>> Sorry, I did not explain it clear enough before.
>>
>> Jeff is right. The NX fix is introduced in this patch series.
>> The reason is that when we update page table to protect the SMRAM, we would like enable the protection as early as possible.
>> We moved the NX enabling code from C function to ASM function to achieve that.
>>
>> You can see my GIT message in 5/6.
>> ==============
>> The XD enabling code is moved to SmiEntry to let NX take effect.
>> ==============
>>
>> Unfortunately, I introduced a bug there. You help to catch it and now I fix it.
>> It is not related to current code.
>>
>> What about your idea?
> 
> Right, I missed that the XD handling in SmiEntry.nasm was brand new code
> added by this series. So, the current structure of both series should be
> fine; I should be able to start testing them (hopefully) soon.

Jeff's v2 series was fine, and it is now committed (see
<https://bugzilla.tianocore.org/show_bug.cgi?id=216#c2>).

However, in order to test it successfully with OVMF, I needed the
patches linked in <https://bugzilla.tianocore.org/show_bug.cgi?id=230>.
I intend to test this v3 series too with those (QEMU and OVMF) patches
applied. I hope I can get to it tomorrow or so.

Thanks!
Laszlo


>> From: Fan, Jeff
>> Sent: Friday, November 11, 2016 8:38 PM
>> To: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
>> Cc: Tian, Feng <feng.tian@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH V3 0/6] Enable SMM page level protection.
>>
>> Laszlo,
>>
>> NX support and fix in SmiEntry.asm is new code for UefiCpuPkg. So, that means we still have other unknown problem on S3 boot issue.
>>
>> Jeff
>>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, November 11, 2016 8:26 PM
>> To: Yao, Jiewen; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Fan, Jeff
>> Cc: Tian, Feng; Kinney, Michael D; Paolo Bonzini; Zeng, Star
>> Subject: Re: [edk2] [PATCH V3 0/6] Enable SMM page level protection.
>>
>> Jiewen, Jeff,
>>
>> On 11/11/16 10:12, Yao, Jiewen wrote:
>>> HI Laszlo
>>> I fixed the IA32 boot issue in this patch
>>
>> Thank you.
>>
>> Jiewen, I'd like to request the following:
>>
>> - Please separate the fix for the incorrect parameter passing to
>>   SmiRendezvous() out to a separate patch. It is my understanding that
>>   the issue exists already in the master branch. Is that right? If it
>>   is, then the fix should not be tied to the SMM page level protection
>>   feature.
>>
>> - Please give that patch to Jeff.
>>
>> Jeff,
>>
>> can you please repost your series
>>
>>   [edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path
>>
>> to edk2-devel, as v3, with Jiewen's patch from above included, as patch#4? Because, I would like to see a patch series that addresses all known S3 issues that we've uncovered in this investigation.
>>
>> The first three patches should fix BZ#216, yes.
>>
>> The last (4th) patch, from Jiewen, is unrelated to that BZ indeed, but it nonetheless addresses an existent issue in PiSmmCpuDxeSmm that can be hit during S3.
>>
>> My goal is to apply that series (the first 3 patches from Jeff, and the fourth patch from Jiewen), and to test it as one unit. I'd like to see if those changes fix the infrequent, but still triggerable issues with
>> S3+SMM, for both Ia32 and Ia32X64. If everything works fine, then that
>> series should be committed.
>>
>> After that, I'd like to test Jiewen's v4 series for the SMM page level protection, separately.
>>
>> In other words, first we should fix the existent bugs that Jiewen's SMM page level protection feature only amplifies (but doesn't introduce) on QEMU/KVM + OVMF. Once the known bugs are fixed, I'll be glad to test the new feature.
>>
>> Would this work for you guys?
>>
>> Thank you,
>> Laszlo
>>
>>> with DEBUG message update you suggested.
>>>
>>> My unit test failed before. Now it can pass.
>>> I validated on a real IA32 and Windows OVMF with and without XD.
>>>
>>>
>>> For QEMU installation, it is still on progress.
>>> We have setup a Fedora 24 host, download QEMU, and install it.
>>> But we are still struggling to make QEMU boot on Fedora.
>>> Your step by step is great. There is still some minor place we stuck in due to my ignorance.
>>> My goal is still to setup an environment like yours for our validation or issue reproduce.
>>> It just need take some time, more than I expected. sign...
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>>>> Of Jiewen Yao
>>>> Sent: Friday, November 11, 2016 5:01 PM
>>>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>>> Cc: Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Kinney, Michael D
>>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Paolo Bonzini <pbonzini@redhat.com<mailto:pbonzini@redhat.com>>;
>>>> Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Fan, Jeff <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>;
>>>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>>> Subject: [edk2] [PATCH V3 0/6] Enable SMM page level protection.
>>>>
>>>>
>>>> ==== below is V3 description ====
>>>> 1) PiSmmCpu: Fix CpuIndex corruption issue due to stack malposition.
>>>> (Many thanks to Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> for catching it.)
>>>> 2) PiSmmCpu: Add ASSERT for CpuIndex check.
>>>> 3) PiSmmCpu: Use DEBUG_VERBOSE for page table update.
>>>> 4) PiSmmCpu: Do not report DEBUG message for Ap non present when
>>>> PcdCpuSmmSyncMode==1 (Relex mode).
>>>> 5) PiSmmCpu: Do not report DEBUG message for AP removed when
>>>> PcdCpuHotPlugSupport==TRUE.
>>>>
>>>> Tested combination:
>>>> 1) XD disabled
>>>> 2) XD enabled in SMM and disabled in non-SMM.
>>>> 3) XD enabled in SMM and enabled in non-SMM.
>>>>
>>>> ==== below is V2 description ====
>>>> 1) PiSmmCpu: resolve OVMF multiple processors boot hang issue.
>>>> 2) PiSmmCpu: Add debug info on StartupAp() fails.
>>>> 3) PiSmmCpu: Add ASSERT for AllocatePages().
>>>> 4) PiSmmCpu: Add protection detail in commit message.
>>>> 5) UefiCpuPkg.dsc: Add page table footprint info in commit message.
>>>>
>>>> ==== below is V1 description ====
>>>> This series patch enables SMM page level protection.
>>>> Features are:
>>>> 1) PiSmmCore reports SMM PE image code/data information in
>>>> EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
>>>> 2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable and set XD for
>>>> data page and RO for code page.
>>>> 3) PiSmmCpu enables Static Paging for X64 according to
>>>> PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G is
>>>> used as long as it is supported.
>>>> 4) PiSmmCpu sets importance data structure to be read only, such as
>>>> Gdt, Idt, SmmEntrypoint, and PageTable itself.
>>>>
>>>> tested platform:
>>>> 1) Intel internal platform (X64).
>>>> 2) EDKII Quark IA32
>>>> 3) EDKII Vlv2  X64
>>>> 4) EDKII OVMF IA32 and IA32X64. (with -smp 8)
>>>>
>>>> Cc: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>
>>>> Cc: Feng Tian <feng.tian@intel.com<mailto:feng.tian@intel.com>>
>>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com<mailto:pbonzini@redhat.com>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>>>
>>>> Jiewen Yao (6):
>>>>   MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
>>>>   MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
>>>>   MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
>>>>   UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
>>>>   UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
>>>>   QuarkPlatformPkg/dsc: enable Smm paging protection.
>>>>
>>>>  MdeModulePkg/Core/PiSmmCore/Dispatcher.c               |   66 +
>>>>  MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c    | 1509
>>>> ++++++++++++++++++++
>>>>  MdeModulePkg/Core/PiSmmCore/Page.c                     |  775
>>>> +++++++++-
>>>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.c                |   40
>>>> +
>>>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.h                |   91
>>>> ++
>>>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf              |    2
>>>> +
>>>>  MdeModulePkg/Core/PiSmmCore/Pool.c                     |   16
>>>> +
>>>>  MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h |   51 +
>>>>  MdeModulePkg/MdeModulePkg.dec                          |
>>>> 3 +
>>>>  QuarkPlatformPkg/Quark.dsc                             |    6 +
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c               |   71
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S              |   75
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm            |   75
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm           |   79
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S          |  226
>>>> +--
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm        |   36
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm       |   36
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c          |   37
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c        |    4
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c                  |  135
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c             |
>>>> 144 +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h             |
>>>> 156 +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf           |
>>>> 5 +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c     |
>>>> 871 +++++++++++
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c                 |   39
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h                 |   15
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c                |  274
>>>> +++-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S               |   59
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm             |   62
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm            |   69
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S           |  250
>>>> +---
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm         |   35
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.nasm        |   31
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c           |   30
>>>> +-
>>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c         |    7
>>>> +-
>>>>  UefiCpuPkg/UefiCpuPkg.dec                              |    8 +
>>>>  36 files changed, 4585 insertions(+), 803 deletions(-)  create mode
>>>> 100644 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
>>>>  create mode 100644
>>>> MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h
>>>>  create mode 100644
>>>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>>>
>>>> --
>>>> 2.7.4.windows.1
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>