Re: [Patch v3 00/40] MP Initialize Library

[Laszlo Ersek <lersek@redhat.com>]

On 07/29/16 10:57, Fan, Jeff wrote:
> Laszlo,
> 
> I sent one evaluate patch for you by adding back GDT table load in CpuDxe. Could you please help to verify if it could fix IA32 S3 issue?

Yes, I'll try to look into it shortly. I'll have to retest the other
cases as well.

Thanks!
Laszlo

> Another solution is to remove hardcode from PiSmmCpuDxeSmm driver. But I do not prefer to do it this time. :-)
> 
> Thanks!
> Jeff
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, July 28, 2016 11:21 PM
> To: Fan, Jeff
> Cc: edk2-devel@ml01.01.org
> Subject: Re: [edk2] [Patch v3 00/40] MP Initialize Library
> 
> On 07/28/16 15:55, Fan, Jeff wrote:
>> Laszlo,
>>
>> Many thanks for your verification. 
>>
>> Your dump information and analysis result are very useful. I guess the issue happened at 
>>    UefiCpuPkg\PiSmmCpuDxeSmm\Ia32\MpFuncs.nasm:80     a32     jmp   dword 0x20:0x0
>>
>> The Proteted mode CS in current GDT table is not 0x20. But the PiSmmCpuDxeSmm hardcode it to 0x20.
> 
> Ah, good point; I recall:
> 
> commit 0d4c1db81aab86963536deb8253f35546c4398ea
> Author: Michael Kinney <michael.d.kinney@intel.com>
> Date:   Fri Oct 30 17:32:27 2015 +0000
> 
>     UefiCpuPkg: CpuDxe: Update GDT to be consistent with DxeIplPeim
> 
> as another related patch.
> 
>> I will try it fix it tomorrow and feedback to you. 
> 
> Thank you, I'll attempt to test it soon after.
> 
> Cheers
> Laszlo
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, July 28, 2016 9:24 PM
>> To: Fan, Jeff; edk2-devel@ml01.01.org
>> Subject: Re: [edk2] [Patch v3 00/40] MP Initialize Library
>>
>> On 07/25/16 04:52, Jeff Fan wrote:
>>> We add MP Initialize Library defined in UefiCpuPkg/Include/Library/MpInitLib.h.
>>> It will provide basic functionalities of MP services and could be 
>>> consumed by CPU MP PEI and CPU MP DXE to produce CPU MP PPI and CPU 
>>> MP Protocol. Then most of code could be shared between PEI and DXE modules.
>>>
>>> PeiMpInitLib and DxeMpInitLib are added to make the CpuMpPei and 
>>> CpuDxe more simply.
>>>
>>> I also updated the Ovmf Platform and Quark platform to consume MP 
>>> Initialize library. I have tested Ovmf platform and have not tested Quark yet.
>>>
>>> v3:
>>>   1. Update Patch #2, #4 - #8, #28, #33, #36, #38 per Giri's comments to
>>>      a. Update SDM date to June, 2016
>>>      b. Mention BCD format in CPU_MICROCODE_DATE
>>>      c. Rename ProcessorChecksum to Checksum to match SDM.
>>>      d. Add whitespace after MpInitLibInitialize
>>>      e. Rename MpInitLibSwitchBsp to MpInitLibSwitchBSP to match PI spec.
>>>      f. Rename NumApsExecutingLoction to NumApsExecutingLocation
>>>      g. Add whitespace after ; in .nasm file
>>>      h. Rename *RellocateAp* to *RelocateAp*
>>>   2. Update Patch #16, #17, #29-#32 to
>>>      a. Use CamelCase for mStopCheckAllApsStatus and CheckAndUpdateApsStatus().
>>>   3. Update Patch #36 and #39 to
>>>      a. Add PeiMpInitLib instance in UefiCpuPkg.dsc
>>>      b. Add DxeMpInitLib instance in UefiCpuPkg.dsc
>>>   4. Update Patch #39 and #40 to 
>>>      a. move the code of consuming MP Initialize library from patch #40 to
>>>      patch #39.
>>>   5. Update Patch #1, #3 - #8, #16 to
>>>      a. Add Reviewed-by: Giri P Mudusuru <giri.p.mudusuru@intel.com>
>>>
>>> I fork the whole tree with the updated v3 patches at
>>> https://github.com/vanjeff/edk2/tree/MpInitLibV3 for review.
>>
>> I rebased your branch (originally at 09948b72fbb7) on top of current master (= 39dbc4d55347), and built it. It builds fine.
>>
>> Scenarios that I tested, and seem to work:
>>
>> * Q35, Ia32, with -D SMM_REQUIRE, 4 VCPUs, Fedora guest:
>>   - normal boot path: pass
>>   - S3 resume: FAIL (see details below)
>>
>> * i440fx, X64, without -D SMM_REQUIRE, 8 VCPUs, Fedora guest:
>>   - normal boot path: pass
>>   - S3 resume: pass
>>
>> I re-verified the fix for <https://tianocore.acgmultimedia.com/show_bug.cgi?id=86>, which requires CpuMpPei to actually *work* on both boot paths, and it works fine.
>>
>> * Q35, Ia32X64, with -D SMM_REQUIRE, 4 VCPUs, Fedora guest:
>>   - normal boot path: pass
>>
>> On the normal boot path, I also verified that the MTRR setup was consistent across the VCPUs (otherwise Linux would have complained), plus the fix for <https://tianocore.acgmultimedia.com/show_bug.cgi?id=86> was working fine too. I also checked that the UEFI variable services worked, bound to the BSP, and then bound to the first AP as well. (Using the "taskset" Linux command, with "efibootmgr", to list the variables.) I quickly checked that Secure Boot was still recognized by the guest (Fedora) as enabled.
>>
>>   - S3 resume: pass
>>
>> Repeated the BZ#86 check and the variable access checks from within the resumed guest, all pass.
>>
>> * Q35, Ia32X64, with -D SMM_REQUIRE, 4 VCPUs, Windows 8.1 guest:
>>   - normal boot path: pass
>>
>> On the normal boot path, I checked Secure Boot enablement with PowerShell.
>>
>>   - S3 resume: pass.
>>
>> Now, about the one failure case. QEMU logs the following:
>>
>>> KVM internal error. Suberror: 3
>>> KVM internal error. Suberror: 3
>>> KVM internal error. Suberror: 3
>>> extra data[0]: 80000b0d
>>> extra data[0]: 80000b0d
>>> extra data[1]: 31
>>> extra data[1]: 31
>>> extra data[0]: 80000b0d
>>> extra data[1]: 31
>>>
>>> EAX=60000011 EBX=00000000 ECX=00000000 EDX=0009f000
>>> ESI=000000b5 EDI=00000000 EBP=00000000 ESP=00000000
>>> EIP=00000032 EFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES
>>> =9f00 0009f000 0000ffff 00009300 DPL=0 DS16 [-WA] CS =9f00 0009f000 
>>> 0000ffff 00009b00 DPL=0 CS16 [-RA] SS =9f00 0009f000 0000ffff 
>>> 00009300
>>> DPL=0 DS16 [-WA] DS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA] 
>>> FS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA] GS =0000 
>>> 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
>>> LDT=0000 00000000 0000ffff 00008200 DPL=0 LDT TR =0000 00000000 
>>> 0000ffff 00008b00 DPL=0 TSS32-busy
>>> GDT=     7f2d8000 00000017
>>> IDT=     7f2d8018 000007ff
>>> CR0=60000011 CR2=00000000 CR3=00000000 CR4=00000000
>>> DR0=00000000 DR1=00000000 DR2=00000000 DR3=00000000
>>> DR6=ffff0ff0 DR7=00000400
>>> EFER=0000000000000000
>>> Code=00 2e 66 0f 01 1c 31 c0 8e d8 0f 20 c0 66 83 c8 01 0f 22 c0 <66>
>>> 67 ea 3b f0 09 00 20 00 66 b8 08 00 66 8e d8 66 8e c0 66 8e e0 66 8e
>>> e8 66 8e d0 89 d6
>>>
>>> EAX=60000011 EBX=00000000 ECX=00000000 EDX=0009f000
>>> ESI=000000b5 EDI=00000000 EBP=00000000 ESP=00000000
>>> EIP=00000032 EFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES
>>> =9f00 0009f000 0000ffff 00009300 DPL=0 DS16 [-WA] CS =9f00 0009f000 
>>> 0000ffff 00009b00 DPL=0 CS16 [-RA] SS =9f00 0009f000 0000ffff 
>>> 00009300
>>> DPL=0 DS16 [-WA] DS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA] 
>>> FS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA] GS =0000 
>>> 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
>>> LDT=0000 00000000 0000ffff 00008200 DPL=0 LDT TR =0000 00000000 
>>> 0000ffff 00008b00 DPL=0 TSS32-busy
>>> GDT=     7f2d8000 00000017
>>> IDT=     7f2d8018 000007ff
>>> CR0=60000011 CR2=00000000 CR3=00000000 CR4=00000000
>>> DR0=00000000 DR1=00000000 DR2=00000000 DR3=00000000
>>> DR6=ffff0ff0 DR7=00000400
>>> EFER=0000000000000000
>>> Code=00 2e 66 0f 01 1c 31 c0 8e d8 0f 20 c0 66 83 c8 01 0f 22 c0 <66>
>>> 67 ea 3b f0 09 00 20 00 66 b8 08 00 66 8e d8 66 8e c0 66 8e e0 66 8e
>>> e8 66 8e d0 89 d6
>>>
>>> EAX=60000011 EBX=00000000 ECX=00000000 EDX=0009f000
>>> ESI=000000b5 EDI=00000000 EBP=00000000 ESP=00000000
>>> EIP=00000032 EFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES
>>> =9f00 0009f000 0000ffff 00009300 DPL=0 DS16 [-WA] CS =9f00 0009f000 
>>> 0000ffff 00009b00 DPL=0 CS16 [-RA] SS =9f00 0009f000 0000ffff 
>>> 00009300
>>> DPL=0 DS16 [-WA] DS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA] 
>>> FS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA] GS =0000 
>>> 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
>>> LDT=0000 00000000 0000ffff 00008200 DPL=0 LDT TR =0000 00000000 
>>> 0000ffff 00008b00 DPL=0 TSS32-busy
>>> GDT=     7f2d8000 00000017
>>> IDT=     7f2d8018 000007ff
>>> CR0=60000011 CR2=00000000 CR3=00000000 CR4=00000000
>>> DR0=00000000 DR1=00000000 DR2=00000000 DR3=00000000
>>> DR6=ffff0ff0 DR7=00000400
>>> EFER=0000000000000000
>>> Code=00 2e 66 0f 01 1c 31 c0 8e d8 0f 20 c0 66 83 c8 01 0f 22 c0 <66>
>>> 67 ea 3b f0 09 00 20 00 66 b8 08 00 66 8e d8 66 8e c0 66 8e e0 66 8e
>>> e8 66 8e d0 89 d6
>>
>> I didn't try to analyze this in depth, but from the hex-encoded instruction stream, it looks like KVM dislikes the 0x66 prefix in front of a JMP instruction (EA, if I recall correctly). Given that this is logged all at once for three processors (out of 4 -- see the description of that test case above), I believe one of the far jumps in the AP mode switching code is incorrect, on the S3 resume path, for Ia32 + SMM.
>>
>> Perhaps one of the "a32" or "o32" NASM prefixes should be tweaked. Similar examples: <https://www.mail-archive.com/edk2-devel@lists.01.org/msg13258.html>, <https://www.mail-archive.com/edk2-devel@lists.01.org/msg13259.html>.
>>
>> Thanks!
>> Laszlo
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>