Hi Laszlo Comments below: -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Friday, November 27, 2015 5:57 AM To: Yao, Jiewen; Kinney, Michael D; edk2-de...@ml01.01.org Cc: Paolo Bonzini; Fan, Jeff Subject: Re: [edk2] [patch 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0.
On 11/25/15 22:56, Yao, Jiewen wrote: > Hi Mike > Thanks for the suggestion. > Previously, I just want to *ADD* without touch old logic. I will use your way > to do it. > > -----Original Message----- > From: Kinney, Michael D > Sent: Thursday, November 26, 2015 2:01 AM > To: Yao, Jiewen; edk2-de...@ml01.01.org; Kinney, Michael D > Cc: Fan, Jeff > Subject: RE: [patch 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0. > > Jiewen, > > For IA32 assembly, can we combine into a single OR instruction that sets both > page enable and WP? > For X64, does it make sense to use single OR instruction instead of 2 BTS > instructions as well? > > With those updates, > > Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> > > Thanks, > > Mike > >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Wednesday, November 25, 2015 4:35 AM >> To: edk2-de...@ml01.01.org >> Cc: Yao, Jiewen <jiewen....@intel.com>; Fan, Jeff >> <jeff....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com> >> Subject: [patch 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0. >> >> So that we can use write-protection for code later. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: "Yao, Jiewen" <jiewen....@intel.com> >> Cc: "Fan, Jeff" <jeff....@intel.com> >> Cc: "Kinney, Michael D" <michael.d.kin...@intel.com> >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 1 + >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 1 + >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 1 + >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 1 + >> 4 files changed, 4 insertions(+) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S >> index 7e1787c..8e64ce8 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S >> @@ -142,6 +142,7 @@ L13: >> >> movl %cr0, %ebx >> orl $0x080000000, %ebx # enable paging >> + orl $0x000010000, %ebx # set WP >> movl %ebx, %cr0 >> leal DSC_OFFSET(%edi),%ebx >> movw DSC_DS(%ebx),%ax >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm >> index e6af344..a955785 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm >> @@ -148,6 +148,7 @@ gSmiCr3 DD ? >> >> mov ebx, cr0 >> or ebx, 080000000h ; enable paging >> + or ebx, 000010000h ; set WP >> mov cr0, ebx >> lea ebx, [edi + DSC_OFFSET] >> mov ax, [ebx + DSC_DS] >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S >> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S >> index 1d40819..8050a00 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S >> @@ -163,6 +163,7 @@ NxeDone: >> wrmsr >> movq %cr0, %rbx >> btsl $31, %ebx >> + btsl $16, %ebx # set WP >> movq %rbx, %cr0 >> retf >> LongMode: # long mode (64-bit code) starts >> here >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm >> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm >> index 6e1d3f1..db170d6 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm >> @@ -159,6 +159,7 @@ Base: >> wrmsr >> mov rbx, cr0 >> bts ebx, 31 >> + bts ebx, 16 ; set WP >> mov cr0, rbx >> retf >> @LongMode: ; long mode (64-bit code) starts >> here >> -- >> 1.9.5.msysgit.0 This patch (= SVN r18960) breaks the 32-bit (Ia32) build of the OVMF SMM series. KVM "rejects" the guest soon after the SMBASE relocation. These are the last messages from the OVMF debug log: > Loading SMM driver at 0x0007FFD2000 EntryPoint=0x0007FFD2253 > PiSmmCpuDxeSmm.efi SMRR Base: 0x7F800000, SMRR Size: 0x800000 > PcdCpuSmmCodeAccessCheckEnable = 1 SMRAM TileSize = 0x00002000 > (0x00001000, 0x00001000) SMRAM SaveState Buffer (0x7FFC4000, > 0x0000E000) CPU[000] APIC ID=0000 SMBASE=7FFBC000 > SaveState=7FFCBC00 Size=00000400 CPU[001] APIC ID=0002 > SMBASE=7FFBE000 SaveState=7FFCDC00 Size=00000400 CPU[002] APIC > ID=0003 SMBASE=7FFC0000 SaveState=7FFCFC00 Size=00000400 CPU[003] > APIC ID=0001 SMBASE=7FFC2000 SaveState=7FFD1C00 Size=00000400 > InstallProtocolInterface: 26EEB3DE-B689-492E-80F0-BE8BD7DA4BA7 > 7FFE0FD0 SMM IPL registered SMM Entry Point address 7FFF19AE > SmmInstallProtocolInterface: EB346B97-975F-4A9F-8B22-F8E92BB3D569 > 7FFE0F68 > SmmInstallProtocolInterface: 1D202CAB-C8AB-4D5C-94F7-3CFCC0D3D335 > 7FFE0FDC SMM S3 SMRAM Structure = 7F5816C0 SMM S3 Structure = 7F800000 > SMM CPU Module exit from SMRAM with EFI_SUCCESS SMM IPL closed SMRAM > window QEMU logs the following: > KVM: entry failed, hardware error 0x80000021 > > If you're running a guest on an Intel machine without unrestricted > mode support, the failure can be most likely due to the guest entering > an invalid state for Intel VT. For example, the guest maybe running in > big real mode which is not supported on less recent Intel processors. > > EAX=00000668 EBX=80010033 ECX=80200001 EDX=0f89fbff > ESI=7f714158 EDI=7ffbc000 EBP=7ffb6000 ESP=7ffbdffc EIP=7ffc40ce > EFL=00000086 [--S--P-] CPL=0 II=0 A20=1 SMM=1 HLT=0 > ES =0020 00000000 ffffffff 00c09300 DPL=0 DS [-WA] > CS =0008 00000000 ffffffff 00c09b00 DPL=0 CS32 [-RA] > SS =0020 00000000 ffffffff 00c09300 DPL=0 DS [-WA] > DS =0020 00000000 ffffffff 00c09300 DPL=0 DS [-WA] > FS =0020 00000000 ffffffff 00c09300 DPL=0 DS [-WA] > GS =0020 00000000 ffffffff 00c09300 DPL=0 DS [-WA] > LDT=0000 00000000 0000ffff 00008200 DPL=0 LDT TR =0000 00000000 > 0000ffff 00008b00 DPL=0 TSS32-busy > GDT= 7ffb6000 0000003f > IDT= 7ffe11b8 000000ff > CR0=80010033 CR2=00000000 CR3=7ffb7000 CR4=00000668 > DR0=00000000 DR1=00000000 DR2=00000000 DR3=00000000 > DR6=ffff0ff0 DR7=00000400 > EFER=0000000000000000 > Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? <??> ?? ?? > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? > ?? (My host does support unrestricted guest, just for clarity's sake.) The relevant snippet from the KVM trace is as follows: > qemu-system-i38-4063 [001] 21044.710895: kvm_entry: vcpu 0 > qemu-system-i38-4063 [001] 21044.710896: kvm_exit: reason > CR_ACCESS rip 0x8045 info 300 0 > qemu-system-i38-4063 [001] 21044.710896: kvm_cr: cr_write 0 = > 0x33 > qemu-system-i38-4063 [001] 21044.710897: kvm_entry: vcpu 0 > qemu-system-i38-4063 [001] 21044.710897: kvm_exit: reason > CR_ACCESS rip 0x7ffc4078 info 3 0 > qemu-system-i38-4063 [001] 21044.710897: kvm_cr: cr_write 3 = > 0x7ffb7000 > qemu-system-i38-4063 [001] 21044.710898: kvm_entry: vcpu 0 > qemu-system-i38-4063 [001] 21044.710898: kvm_exit: reason CPUID > rip 0x7ffc4080 info 0 0 > qemu-system-i38-4063 [001] 21044.710899: kvm_cpuid: func 1 rax > 6e8 rbx 800 rcx 80200001 rdx f89fbff > qemu-system-i38-4063 [001] 21044.710899: kvm_entry: vcpu 0 > qemu-system-i38-4063 [001] 21044.710899: kvm_exit: reason > CR_ACCESS rip 0x7ffc40bf info 4 0 > qemu-system-i38-4063 [001] 21044.710899: kvm_cr: cr_write 4 = > 0x668 > qemu-system-i38-4063 [001] 21044.710901: kvm_entry: vcpu 0 > qemu-system-i38-4063 [001] 21044.710901: kvm_exit: reason > CR_ACCESS rip 0x7ffc40cb info 300 0 > qemu-system-i38-4063 [001] 21044.710901: kvm_cr: cr_write 0 = > 0x80010033 <--------------- this one > qemu-system-i38-4063 [001] 21044.710904: kvm_entry: vcpu 0 > qemu-system-i38-4063 [001] 21044.710904: kvm_exit: reason > UNKNOWN rip 0x7ffc40ce info 2 0 > qemu-system-i38-4063 [001] 21044.710937: kvm_userspace_exit: reason > KVM_EXIT_FAIL_ENTRY (9) [Jiewen] Do you mean KVM reject SMM write BIT16 of CR0 ? It is odd, because my patch sets W+P bit page table entries. ... I now it sounds a little bit paranoid, but can we *please* suspend the churn in the SMM infrastructure in edk2 until the OVMF series goes in? Even if the above edk2 change is otherwise valid, and it turns out to be a KVM or QEMU issue? [Jiewen] OK, I can suspend for a while. Would you mind to let me know your total plan? The above breakage is just one thing. Today I had to adapt the series to SVN r18958 as well (otherwise it wouldn't even build). I knew about the two new SmmCpuFeaturesLib functions introduced by that commit, yes. What I didn't expect was that they would force me to *re-sync* Paolo's patches: OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits -- which he originally contributed on October 13th -- with the *fresh* library instance in UefiCpuPkg. And *that* was utter pain. ... And now apparently Jordan wants me to delay the series even longer, until the S3Resume2Pei module is fixed to work on X64 + SMM. [Jiewen] I do not understand. Why is SMM change related to S3ResumePei??? You can enable IA32 version + IA32/X64 version at first, then enable X64 version. Most real big core platforms have IA32/X64, some embedded small core platforms have IA32 only. But I do not see a real platform enable X64 version S3Resume2Pei, at least at current point of time. Maybe there might be some weird bugs in it, and we need spend long time to debug. I respect your great work on OVMF, and it is good to have a virtual SMM support. If my patch introduces a bug to break virtual SMM, we definitely need fix. I am sorry that I have few knowledge of QUME. If you can give me more hint of why CR0.WP cause failure that would be great. If possible, please, let me know how long do you expect me to wait? If it is 1 week or 2 weeks, I am fine to wait for your progress. At same time, I do hope we can do things step by step. If we need make everything ready and check in together, that will block other progress. Here I strongly recommend you check in IA32 and IA32/X64 support at first. Then we can continue work on X64. On one hand, I can't be thankful enough to Mike and others who have immensely helped with this. On the other hand, this half year plus long experience has been terrible. In the future, please CC me on *all* SMM patches, and give me a chance to test them with OVMF / QEMU, before committing them. [Jiewen] Sure. I assume you can see this patch because you are in edk2-de...@ml01.01.org, and I assume you will review. That is why I just CC module owner only, by default. If you want to be there, I am glad to put your name too. Laszlo _______________________________________________ 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
