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

[Laszlo Ersek <lersek@redhat.com>]

On 11/04/16 10:35, Yao, Jiewen wrote:
> Hi Laszlo
> I just send out V2 patch.
> 
> The new update resolved OVMF ASSERT issue, and added more information in commit message.

Thanks! I'll try to look at v2 (with some more testing) next week.

> 
> However, there is no update for OVMF stability issue. I have no much idea on what happened.
> 
> If you want to try V2 with more tests, that is great.
> But if you want to resolve S3 stability issue at first, I am also OK.
> 
> At same time, if you have any clue on the S3 stability issue, please let me know.

Right... I hope I can ask Paolo and Radim to help us with that.

Thanks
Laszlo

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, November 4, 2016 5:43 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Tian, Feng <feng.tian@intel.com>; Radim Krčmář <rkrcmar@redhat.com>; edk2-devel@ml01.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 0/6] Enable SMM page level protection.
> 
> On 11/03/16 07:53, Jiewen Yao wrote:
>> 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.
> 
> Did you use a Windows QEMU binary for the OVMF tests?
> 
> Please find my test results below, all done on KVM
> (3.10.0-514.el7.x86_64, if anyone is curious :)).
> 
>>
>> 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>>
>> 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.
> 
> Legend:
> 
> - "untested" means the test was not executed because the same test
>   failed or proved unreliable in a less demanding configuration already,
> 
> - "n/a" means a setting or test case was impossible,
> 
> - "fail" and "unreliable" (lower case) are outside the scope of this
>   series; they either capture the pre-series status, or are expected
>   even with the series applied due to the pre-series status,
> 
> - "FAIL" and "UNRELIABLE" mean regressions caused (or exposed) by the
>   series.
> 
> In all cases, 36 bits were used as address width in the CPU HOB (--> up
> to 64GB guest-phys address space).
> 
>    series  OVMF                                                              VCPU     boot       S3 resume
>  # applied platform PcdCpuMaxLogicalProcessorNumber PcdCpuSmmStaticPageTable topology result     result
> -- ------- -------- ------------------------------- ------------------------ -------- ------     ---------
>  1 no      Ia32      64                              n/a                     1x2x2    pass       unreliable
>  2 no      Ia32     255                              n/a                     52x2x2   pass       untested
>  3 no      Ia32     255                              n/a                     53x2x2   unreliable untested
>  4 no      Ia32X64   64                              n/a                     1x2x2    pass       pass
>  5 no      Ia32X64  255                              n/a                     52x2x2   pass       pass
>  6 no      Ia32X64  255                              n/a                     54x2x2   fail       n/a
>  7 yes     Ia32      64                              FALSE                   1x2x2    FAIL       n/a
>  8 yes     Ia32      64                              TRUE                    1x2x2    FAIL       n/a
>  9 yes     Ia32     255                              FALSE                   52x2x2   untested   untested
> 10 yes     Ia32     255                              FALSE                   53x2x2   untested   untested
> 11 yes     Ia32     255                              TRUE                    52x2x2   untested   untested
> 12 yes     Ia32     255                              TRUE                    53x2x2   untested   untested
> 13 yes     Ia32X64   64                              FALSE                   1x2x2    pass       UNRELIABLE
> 14 yes     Ia32X64   64                              TRUE                    1x2x2    FAIL       n/a
> 15 yes     Ia32X64  255                              FALSE                   52x2x2   pass       untested
> 16 yes     Ia32X64  255                              FALSE                   54x2x2   fail       untested
> 17 yes     Ia32X64  255                              TRUE                    52x2x2   untested   untested
> 18 yes     Ia32X64  255                              TRUE                    54x2x2   untested   untested
> 
> Notes for the baseline tests (1-6):
> 
> * test 3: boot unreliable due to out-of-SMRAM (depends on what we do
>   during boot)
> 
> * test 5: S3 wakeup: 1 try, suspend takes very long, CPU intensive;
>   wakeup is quick, works okay
> 
> * test 6: boot fail: out of SMRAM (expected)
> 
> Notes for the tests with the series applied:
> 
> * test 7: normal boot is regressed even with
>   PcdCpuSmmStaticPageTable=FALSE, relative to test 1:
> 
>> SmmInstallProtocolInterface: [EfiSmmReadyToLockProtocol] 0
>> Patch page table start ...
>> Patch page table done!
>> MemoryAttributesTable - NULL
>> SetPageTableAttributes
>> Start...
>> ConvertPageEntryAttribute 0x7FF9A067->0x7FF9A065
>>
>> ASSERT_EFI_ERROR (Status = Invalid Parameter)
>> ASSERT UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c(468): !EFI_ERROR (Status)
> 
>   Note that the processor model used in this test has no support for NX.
> 
> * test 8: results identical to those of test 7
> 
> * test 13: S3 resume is unreliable (regression relative to test 4). End
>   of OVMF log (Fedora guest):
> 
>> S3_BOOT_SCRIPT_LIB_TERMINATE_OPCODE
>> S3BootScriptDone - Success
>> Call AsmDisablePaging64() to return to S3 Resume in PEI Phase
>> Install PPI: [PeiPostScriptTablePpi]
>> Install PPI: [EfiEndOfPeiSignalPpi]
>> Notify: PPI Guid: [EfiEndOfPeiSignalPpi], Peim notify entry point: 857895
>> PeiMpInitLib: CpuMpEndOfPeiCallback () invoked
>> Transfer to 16bit OS waking vector - 9A1D0
> 
>   QEMU log:
> 
>> KVM internal error. Suberror: 1
>> KVM internal error. Suberror: 1
>> emulation failure
>> emulation failure
>> RAX=0000000000000001 RBX=0000000000000000 RCX=000000007ffdbf48 RDX=000000007fede058
>> RSI=0000000000000004 RDI=000000007fede040 RBP=0000000000000000 RSP=000000007e19f000
>> R8 =0000000000000000 R9 =0000000000000000 R10=0000000000000000 R11=0000000000000000
>> R12=0000000000000000 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
>> RIP=000000000009f0fd RFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
>> SS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> DS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> FS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> GS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
>> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
>> GDT=     000000007f294000 00000047
>> IDT=     000000007f294048 00000fff
>> CR0=e0000011 CR2=0000000000000000 CR3=000000007ff80000 CR4=00000220
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000500
>> Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? <??> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??
>> RAX=0000000000000002 RBX=0000000000000000 RCX=000000007ffdbf48 RDX=000000007fede070
>> RSI=0000000000000004 RDI=000000007fede058 RBP=0000000000000000 RSP=000000007e1a7000
>> R8 =0000000000000000 R9 =0000000000000000 R10=0000000000000000 R11=0000000000000000
>> R12=0000000000000000 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
>> RIP=000000000009f0fd RFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
>> SS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> DS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> FS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> GS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
>> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
>> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
>> GDT=     000000007f294000 00000047
>> IDT=     000000007f294048 00000fff
>> CR0=e0000011 CR2=0000000000000000 CR3=000000007ff80000 CR4=00000220
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000500
>> Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? <??> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??
> 
>   This happened 1 time out of the 2 times I ran the test.
> 
> * test 14: boot regression relative to test 4. The symptoms are
>   identical to those seen in test 7, that is:
> 
>> SmmInstallProtocolInterface: [EfiSmmReadyToLockProtocol] 0
>> Patch page table start ...
>> Patch page table done!
>> MemoryAttributesTable - NULL
>> SetPageTableAttributes
>> Start...
>> ConvertPageEntryAttribute 0x7FF98067->0x7FF98065
>>
>> ASSERT_EFI_ERROR (Status = Invalid Parameter)
>> ASSERT UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c(468): !EFI_ERROR (Status)
> 
> 
> Summary:
> 
> - this series seems to break the boot with the Ia32 build of OVMF,
>   regardless of PcdCpuSmmStaticPageTable (cases 7 and 8),
> 
> - it seems to break the boot with the Ia32X64 build of OVMF, with
>   PcdCpuSmmStaticPageTable=TRUE (case 14),
> 
> - for the Ia32X64 build, with PcdCpuSmmStaticPageTable=FALSE, boot
>   works, but S3 becomes unreliable (case 13),
> 
> - because there was no successful boot with
>   PcdCpuSmmStaticPageTable=TRUE, I couldn't check if and how the
>   increased SMRAM demand would impact the maximum count of bootable
>   VCPUs. (For this test, I should likely increase the guest-phys address
>   width from 36 bits anyway.)
> 
> Thanks
> Laszlo
> 
> _______________________________________________
> 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
>