Comments below: -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Friday, November 27, 2015 9:59 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/27/15 02:14, Yao, Jiewen wrote: > 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 ? Yes, that seems to be the case. As soon as the WP bit is set in CR0, KVM cannot continue executing the guest > It is odd, because my patch sets W+P bit page table entries. It can very well be a problem in KVM itself -- I hope Paolo can help out with analyzing that. My problem is that it is practically impossible to work with such a large feature if there are no stable parts, only moving parts. Usually while working on a feature, I don't rebase very frequently (exactly for the reason mentioned above). I also don't delay rebasing indefinitely (because then the breakage can be very large and hard to bisect). I've run into this right now because I've started preparing version 5 of the OVMF SMM series for posting, and for that I definitely need to rebase on master. > > ... 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? Thanks. My plan is that I will post v5 in a few minutes (or in one hour). That version will need reviews for three patches *only*. So I hope those will arrive quickly. Beginning of next week I would like to commit the series to SVN, and then I won't be blocking you any longer. I hope this is tolerable. [Jiewen] Sounds great! That would be helpful. Thank you very much. I will suspend my check in until next week, after you check in your SMM series. > > 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??? Sorry, the way I put it was not easy to understand. The SMM change in this patch is not related to S3Resume2Pei at all. My point was that the longer I'm forced to delay committing this series, there will be more and more parallel changes to other things in the edk2 tree that will interfere with this work. And that turns into a never-ending race for me: given that my work is not in the tree yet, but the *other* change *is*, the burden is on *me only* to track down and isolate the breakage. This is why I'm requresting that SMM infrastructure changes be halted for a *little bit*, so that my work can finally converge, and get in the tree. [Jiewen] I agree with you. It is painful to find conflict when rebase or check in. I do not have any patch to *CHANGE* interface recently. SmmCpuFeatureLib.h is the only one I touched recently. All my other changes will be self-contained, and should not impact SmmCpuFeatureLib. Once it is inside the tree, and a patch like this breaks it (for whatever reason, really), then we'll have a *shared* responsibility to track down the regression. [Jiewen] I agree with you. We can work together (maybe with help from Paolo, too) to trace the root-cause. > You can enable IA32 version + IA32/X64 version at first, then enable X64 > version. Absolutely. I agree. > 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. We can definitely do this later. Actually, we *should* do this later. [Jiewen] Agree. We never validate that just because there is no platform support. It is good news that OVMF has such capability. We should do that to make infrastructure complete. Maybe a real platform has X64 PEIM in the future, who knows. :-) The *only* reason I brought up the S3Resume2Pei question because Jordan's suggestion about it would compel me to wait *even longer* with this work. > 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. Thank you. Again, it is possible that the patch is perfectly valid, and the problem is in the emulator in KVM. However, for my work at the moment, this difference is not relevant. What is relevant is to minimize the number of unstable / moving parts in the *full* software stack, for just a little while, so I can get this darn series into edk2. [Jiewen] Agree. > 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. We should look into this all together, but later. Hopefully next week or so. I hope that Paolo can help us from the KVM side. [Jiewen] Agree, I am ready to help. > 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. If everything goes well, I hope my series can be committed until mid next week. [Jiewen] That is great news. :-) > 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. I agree that we should be able to work in parallel in general. It is an exceptionally unfortunate scenario right now. My series is large and intrusive, and it depends on the new SMM infrastructure in UefiCpuPkg. (You could say that it was one of the motivators, if not *the* motivator, for those new modules there.) At the same time, you are (understandably!) busy developing the same exact infrastructure. Once I'm done with this series, I'll have other stuff to work on (oh God I can't wait for the switch!!!), and then I should be out of your hair. [Jiewen] Yes. We are developing security feature to do more protection for SMM. It is unfortunate to conflict with your OVMF work. As far as I know, SmmCpuFeatureLib is the only interface *CHANGE* recently. Any other update is self-contained, and should not impact OVMF SmmCpuFeatureLib design. *Further* breakage is of course possible, but as I said above, then we will have shared responsibility in tracking that down. Plus, it won't be any different from any other kind of regression in edk2 -- and we can mostly deal with that anyway. [Jiewen] Agree. So again, the unique problem here is that the core infrastructure patch is conflicting with a large *pending* series. > Here I strongly recommend you check in IA32 and IA32/X64 support at first. > Then we can continue work on X64. I absolutely agree. > 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. Thanks! > 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. Unfortunately, I have quite limited bandwidth for reviewing everything that I would like to, or should. So I try to decide which patches are *more likely* to cause problems, and which look more "innocent". As you surely remember, I did comment on one of your SMM patches recently -- but I cannot comment on *all* of them. In addition, I might even miss patches on the list, if the traffic is high, or I'm simply very busy. For which reason I'd rather like to regression-test your outstanding patches, going forward. Review would be too much for me, but perhaps I can cover regression testing with OVMF. Just please CC me on the SMM patches that *you* think could be more intrusive. [Jiewen] Oops. I made wrong assumption. I will CC you on all my SMM patch. As a general hint, everything that deals with mode switches, control registers and the like (i.e., wherever you tend to write assembly code in edk2) is a good candidate for causing regressions in a virtual environment. So I would definitely like to be CC'd on such patches. [Jiewen] I agree. Thank you! Laszlo > > > 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
