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

[Laszlo Ersek <lersek@redhat.com>]

Jiewen,

On 11/17/16 02:36, Yao, Jiewen wrote:
> You are right. That is good suggestion to mention it. I will add.
> 
> Thank you
> Yao Jiewen

You forgot to commit patch #1 from the series, and the build is now broken.

... I have now committed patch #1 for you, with the following commit
message modifications (all according to feedback on the list):
- added my T-b
- added Jeff's R-b
- added Mike's R-b
- added Mike's paragraph (visible below) about
  gEdkiiPiSmmMemoryAttributesTableGuid

Commit 97d2760429d6.

Also I see patch #6 (17abe97671ee), for QuarkPlatformPkg, got committed
with my Tested-by. I didn't test that patch, and I even said so -- I
wrote "For patches #1 through #5".

Please be more careful about last minute changes and about applying
feedback tags.

Thanks
Laszlo

> From: Kinney, Michael D
> Sent: Thursday, November 17, 2016 9:31 AM
> To: Fan, Jeff <jeff.fan@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>
> Subject: RE: [PATCH V3 0/6] Enable SMM page level protection.
> 
> Jiewen,
> 
> The new file MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c and the
> new code in MdeModulePkg/Core/PiSmmCore/Page.c are based on the algorithms
> and implementation from MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> and MdeModulePkg/Core/Dxe/Mem/Page.c.
> 
> Also, the new GUID gEdkiiPiSmmMemoryAttributesTableGuid and its associated
> structure are based on the EFI_MEMORY_ATTRIBUTES_TABLE and GUID from the
> UEFI Specification.
> 
> I recommend you update the commit messages to highlight that these new
> components are based on the Memory Attributes Table feature from the UEFI
> Specification and the existing DXE Core implementation that supports that
> feature.
> 
> With those commit message updates, Series
> 
> Reviewed-by: Michael Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> 
> Mike
> 
>> -----Original Message-----
>> From: Fan, Jeff
>> Sent: Wednesday, November 16, 2016 4:45 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>> Cc: Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Kinney,
>> Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Paolo
>> Bonzini <pbonzini@redhat.com<mailto:pbonzini@redhat.com>>
>> Subject: RE: [PATCH V3 0/6] Enable SMM page level protection.
>>
>> Reviewed-by: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>
>>
>> Thanks!
>>
>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Friday, November 11, 2016 5:01 PM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>> Cc: Fan, Jeff; Tian, Feng; Zeng, Star; Kinney, Michael D; Laszlo Ersek; Paolo Bonzini
>> Subject: [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
> https://lists.01.org/mailman/listinfo/edk2-devel
>