summaryrefslogtreecommitdiffstats
path: root/UefiCpuPkg/PiSmmCpuDxeSmm
Commit message (Collapse)AuthorAgeFilesLines
* UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessingKun Qin2021-04-122-2/+9
| | | | | | | | | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3283 Current SMM Save State routine does not check the number of bytes to be read, when it comse to read IO_INFO, before casting the incoming buffer to EFI_SMM_SAVE_STATE_IO_INFO. This could potentially cause memory corruption due to extra bytes are written out of buffer boundary. This change adds a width check before copying IoInfo into output buffer. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20210406195254.1018-2-kuqin12@gmail.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Support detect SMM shadow stack overflowSheng, W2021-04-091-1/+8
| | | | | | | | | | | | | | Use SMM stack guard feature to detect SMM shadow stack overflow. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3280 Signed-off-by: Sheng Wei <w.sheng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com> Cc: Roger Feng <roger.feng@intel.com>
* UefiCpuPkg/PiSmmCpu: Don't allocate Token for SmmStartupThisApRay Ni2021-03-111-7/+23
| | | | | | | | | | | | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3199 When Token points to mSmmStartupThisApToken, this routine is called from SmmStartupThisAp() in non-blocking mode due to PcdCpuSmmBlockStartupThisAp == FALSE. In this case, caller wants to startup AP procedure in non-blocking mode and cannot get the completion status from the Token because there is no way to return the Token to caller from SmmStartupThisAp(). Caller needs to use its specific way to query the completion status. There is no need to allocate a token for such case so the 3 overheads can be avoided: 1. Call AllocateTokenBuffer() when there is no free token. 2. Get a free token from the token buffer. 3. Call ReleaseToken() in APHandler(). Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is not correctedk2-stable202102Sheng Wei2021-03-023-4/+8
| | | | | | | | | | | | | | | | | | | | | | | In function InitGdt(), SmiPFHandler() and Gen4GPageTable(), it uses CpuIndex * mSmmStackSize to get the SMM stack address offset for multi processor. It misses the SMM Shadow Stack Size. Each processor will use mSmmStackSize + mSmmShadowStackSize in the memory. It should use CpuIndex * (mSmmStackSize + mSmmShadowStackSize) to get this SMM stack address offset. If mSmmShadowStackSize > 0 and multi processor enabled, it will get the wrong offset value. CET shadow stack feature will set the value of mSmmShadowStackSize. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3237 Signed-off-by: Sheng Wei <w.sheng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Roger Feng <roger.feng@intel.com> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com>
* UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bitSheng Wei2021-03-021-1/+14
| | | | | | | | | | | | | | | | | | | | | | | | | If CET shadows stack feature enabled in SMM and stack switch is enabled. When code execute from SMM handler to SMM exception, CPU will check SMM exception shadow stack token busy bit if it is cleared or not. If it is set, it will trigger #DF exception. If it is not set, CPU will set the busy bit when enter SMM exception. So, the busy bit should be cleared when return back form SMM exception to SMM handler. Otherwise, keeping busy bit 1 will cause to trigger #DF exception when enter SMM exception next time. So, we use instruction SAVEPREVSSP, CLRSSBSY and RSTORSSP to clear the shadow stack token busy bit before RETF instruction in SMM exception. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3192 Signed-off-by: Sheng Wei <w.sheng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Roger Feng <roger.feng@intel.com> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com>
* UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.cZeng, Star2021-01-111-17/+57
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This patch makes two refinements to reduce SMRAM consumption in CpuS3.c. 1. Only do CopyRegisterTable() when register table is not empty, IsRegisterTableEmpty() is created to check whether the register table is empty or not. Take empty PreSmmInitRegisterTable as example, about 24K SMRAM consumption could be reduced when mAcpiCpuData.NumberOfCpus=1024. sizeof (CPU_REGISTER_TABLE) = 24 mAcpiCpuData.NumberOfCpus = 1024 = 1K mAcpiCpuData.NumberOfCpus * sizeof (CPU_REGISTER_TABLE) = 24K 2. Only copy table entries buffer instead of whole buffer. AllocatedSize in SourceRegisterTableList is the whole buffer size. Actually, only the table entries buffer needs to be copied, and the size is TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY). Take AllocatedSize=0x1000=4096, TableLength=100 and NumberOfCpus=1024 as example, about 1696K SMRAM consumption could be reduced. sizeof (CPU_REGISTER_TABLE_ENTRY) = 24 TableLength = 100 TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY) = 2400 AllocatedSize = 0x1000 = 4096 AllocatedSize - TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY) = 4096 - 2400 = 1696 NumberOfCpus = 1024 = 1K NumberOfCpus * (AllocatedSize - TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY)) = 1696K This patch also corrects the CopyRegisterTable() function description. Signed-off-by: Star Zeng <star.zeng@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Message-Id: <20210111015419.28368-1-star.zeng@intel.com>
* UefiCpuPkg/Feature: Support different thread count per coreRay Ni2020-12-041-27/+46
| | | | | | | | | | | | | | | | | Today's code assumes every core contains the same number of threads. It's not always TRUE for certain model. Such assumption causes system hang when thread count per core is different and there is core or package dependency between CPU features (using CPU_FEATURE_CORE_BEFORE/AFTER, CPU_FEATURE_PACKAGE_BEFORE/AFTER). The change removes such assumption by calculating the actual thread count per package and per core. Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Cc: Yun Lou <yun.lou@intel.com> Acked-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table addressSheng Wei2020-11-184-37/+74
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When trying to get page table base, if mInternalCr3 is zero, it will use the page table from CR3, and reflect the page table depth by CR4 LA57 bit. If mInternalCr3 is non zero, it will use the page table from mInternalCr3 and reflect the page table depth of mInternalCr3 at same time. In the case of X64, we use m5LevelPagingNeeded to reflect the depth of the page table. And in the case of IA32, it will not the page table depth information. This patch is a bug fix when enable CET feature with 5 level paging. The SMM page tables are allocated / initialized in PiCpuSmmEntry(). When CET is enabled, PiCpuSmmEntry() must further modify the attribute of shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry(). So the page table base address is set to mInternalCr3 for modifty the page table attribute. It could not use CR4 LA57 bit to reflect the page table depth for mInternalCr3. So we create a architecture-specific implementation GetPageTable() with 2 output parameters. One parameter is used to output the page table address. Another parameter is used to reflect if it is 5 level paging or not. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015 Signed-off-by: Sheng Wei <w.sheng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typoSheng Wei2020-11-181-5/+5
| | | | | | | | | | | | | | | | Change the variable name from mInternalGr3 to mInternalCr3. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015 Signed-off-by: Sheng Wei <w.sheng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg: Allow AP booting under SEV-ESTom Lendacky2020-08-171-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This sequence is intercepted by the hypervisor, which sets the AP's registers to the values requested by the sequence. At that point, the hypervisor can start the AP, which will then begin execution at the appropriate location. Under SEV-ES, AP booting presents some challenges since the hypervisor is not allowed to alter the AP's register state. In this situation, we have to distinguish between the AP's first boot and AP's subsequent boots. First boot: Once the AP's register state has been defined (which is before the guest is first booted) it cannot be altered. Should the hypervisor attempt to alter the register state, the change would be detected by the hardware and the VMRUN instruction would fail. Given this, the first boot for the AP is required to begin execution with this initial register state, which is typically the reset vector. This prevents the BSP from directing the AP startup location through the INIT-SIPI-SIPI sequence. To work around this, the firmware will provide a build time reserved area that can be used as the initial IP value. The hypervisor can extract this location value by checking for the SEV-ES reset block GUID that must be located 48-bytes from the end of the firmware. The format of the SEV-ES reset block area is: 0x00 - 0x01 - SEV-ES Reset IP 0x02 - 0x03 - SEV-ES Reset CS Segment Base[31:16] 0x04 - 0x05 - Size of the SEV-ES reset block 0x06 - 0x15 - SEV-ES Reset Block GUID (00f771de-1a7e-4fcb-890e-68c77e2fb44e) The total size is 22 bytes. Any expansion to this block must be done by adding new values before existing values. The hypervisor will use the IP and CS values obtained from the SEV-ES reset block to set as the AP's initial values. The CS Segment Base represents the upper 16 bits of the CS segment base and must be left shifted by 16 bits to form the complete CS segment base value. Before booting the AP for the first time, the BSP must initialize the SEV-ES reset area. This consists of programming a FAR JMP instruction to the contents of a memory location that is also located in the SEV-ES reset area. The BSP must program the IP and CS values for the FAR JMP based on values drived from the INIT-SIPI-SIPI sequence. Subsequent boots: Again, the hypervisor cannot alter the AP register state, so a method is required to take the AP out of halt state and redirect it to the desired IP location. If it is determined that the AP is running in an SEV-ES guest, then instead of calling CpuSleep(), a VMGEXIT is issued with the AP Reset Hold exit code (0x80000004). The hypervisor will put the AP in a halt state, waiting for an INIT-SIPI-SIPI sequence. Once the sequence is recognized, the hypervisor will resume the AP. At this point the AP must transition from the current 64-bit long mode down to 16-bit real mode and begin executing at the derived location from the INIT-SIPI-SIPI sequence. Another change is around the area of obtaining the (x2)APIC ID during AP startup. During AP startup, the AP can't take a #VC exception before the AP has established a stack. However, the AP stack is set by using the (x2)APIC ID, which is obtained through CPUID instructions. A CPUID instruction will cause a #VC, so a different method must be used. The GHCB protocol supports a method to obtain CPUID information from the hypervisor through the GHCB MSR. This method does not require a stack, so it is used to obtain the necessary CPUID information to determine the (x2)APIC ID. The new 16-bit protected mode GDT entry is used in order to transition from 64-bit long mode down to 16-bit real mode. A new assembler routine is created that takes the AP from 64-bit long mode to 16-bit real mode. This is located under 1MB in memory and transitions from 64-bit long mode to 32-bit compatibility mode to 16-bit protected mode and finally 16-bit real mode. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: pause in WaitForSemaphore() before re-fetchLaszlo Ersek2020-07-311-7/+11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Most busy waits (spinlocks) in "UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c" already call CpuPause() in their loop bodies; see SmmWaitForApArrival(), APHandler(), and SmiRendezvous(). However, the "main wait" within APHandler(): > // > // Wait for something to happen > // > WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); doesn't do so, as WaitForSemaphore() keeps trying to acquire the semaphore without pausing. The performance impact is especially notable in QEMU/KVM + OVMF virtualization with CPU overcommit (that is, when the guest has significantly more VCPUs than the host has physical CPUs). The guest BSP is working heavily in: BSPHandler() [MpService.c] PerformRemainingTasks() [PiSmmCpuDxeSmm.c] SetUefiMemMapAttributes() [SmmCpuMemoryManagement.c] while the many guest APs are spinning in the "Wait for something to happen" semaphore acquisition, in APHandler(). The guest APs are generating useless memory traffic and saturating host CPUs, hindering the guest BSP's progress in SetUefiMemMapAttributes(). Rework the loop in WaitForSemaphore(): call CpuPause() in every iteration after the first check fails. Due to Pause Loop Exiting (known as Pause Filter on AMD), the host scheduler can favor the guest BSP over the guest APs. Running a 16 GB RAM + 512 VCPU guest on a 448 PCPU host, this patch reduces OVMF boot time (counted until reaching grub) from 20-30 minutes to less than 4 minutes. The patch should benefit physical machines as well -- according to the Intel SDM, PAUSE "Improves the performance of spin-wait loops". Adding PAUSE to the generic WaitForSemaphore() function is considered a general improvement. Cc: Eric Dong <eric.dong@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Ray Ni <ray.ni@intel.com> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1861718 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200729185217.10084-1-lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg: Add New Memory AttributesOleksiy Yakovlev2020-07-151-1/+1
| | | | | | | | | | | | | Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO attributes introduced in UEFI 2.8. (UEFI 2.8, mantis 1919 and 1872). Use attributes bitmasks, defined in MdePkg. Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200702205039.52400-5-oleksiyy@ami.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Tested-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on AMDKirkendall, Garrett2020-07-074-5/+46
| | | | | | | | | | | | | | | | AMD does not support MSR_IA32_MISC_ENABLE. Accessing that register causes and exception on AMD processors. If Execution Disable is supported, but if the processor is an AMD processor, skip manipulating MSR_IA32_MISC_ENABLE[34] XD Disable bit. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Garrett Kirkendall <garrett.kirkendall@amd.com> Message-Id: <20200622131825.1352-5-Garrett.Kirkendall@amd.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Tested-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Remove useless code in ResetTokens.Dong, Eric2020-04-131-20/+0
| | | | | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 After remove Used parameter, below code in ResetTokens can also be removed: 1. The RunningApCount parameter will be reset in GetFreeToken. 2. The ReleaseSpinLock should be called in ReleaseToken function, Code in this function seems like a later fix if ReleaseToken not Release it. We should remove code here and fix the real issue if existed. Signed-off-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Remove Used parameter.Dong, Eric2020-04-132-6/+5
| | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 After patch "UefiCpuPkg/PiSmmCpuDxeSmm: Improve the performance of GetFreeToken()" which adds new parameter FirstFreeToken, it's not need to use Uses parameter. This patch used to remove this parameter. Signed-off-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Improve the performance of GetFreeToken()Ray Ni2020-04-132-47/+27
| | | | | | | | | | | | | | | | | | | | | | | | | | | Today's GetFreeToken() runs at the algorithm complexity of O(n) where n is the size of the token list. The change introduces a new global variable FirstFreeToken and it always points to the first free token. So the algorithm complexity of GetFreeToken() decreases from O(n) to O(1). The improvement matters when some SMI code uses StartupThisAP() service for each of the AP such that the algorithm complexity becomes O(n) * O(m) where m is the AP count. As next steps, 1. PROCEDURE_TOKEN.Used field can be optimized out because all tokens before FirstFreeToken should have "Used" set while all after FirstFreeToken should have "Used" cleared. 2. ResetTokens() can be optimized to only reset tokens before FirstFreeToken. v2: add missing line in InitializeDataForMmMp. v3: update copyright year to 2020. Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Star Zeng <star.zeng@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplugLaszlo Ersek2020-03-041-2/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The "ACPI_CPU_DATA.NumberOfCpus" field is specified as follows, in "UefiCpuPkg/Include/AcpiCpuData.h" (rewrapped for this commit message): // // The number of CPUs. If a platform does not support hot plug CPUs, // then this is the number of CPUs detected when the platform is booted, // regardless of being enabled or disabled. If a platform does support // hot plug CPUs, then this is the maximum number of CPUs that the // platform supports. // The InitializeCpuBeforeRebase() and InitializeCpuAfterRebase() functions in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c" try to restore CPU configuration on the S3 Resume path for *all* CPUs accounted for in "ACPI_CPU_DATA.NumberOfCpus". This is wrong, as with CPU hotplug, not all of the possible CPUs may be present at the time of S3 Suspend / Resume. The symptom is an infinite wait. Instead, the "mNumberOfCpus" variable should be used, which is properly maintained through the EFI_SMM_CPU_SERVICE_PROTOCOL implementation (see SmmAddProcessor(), SmmRemoveProcessor(), SmmCpuUpdate() in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c"). When CPU hotplug is disabled, "mNumberOfCpus" is constant, and equals "ACPI_CPU_DATA.NumberOfCpus" at all times. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Eric Dong <eric.dong@intel.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Cc: Ray Ni <ray.ni@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200226221156.29589-3-lersek@redhat.com> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Reviewed-by: Eric Dong <eric.dong@intel.com> Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> [lersek@redhat.com: shut up UINTN->UINT32 warning from Windows VS2019 PR]
* UefiCpuPkg/PiSmm: Fix various typosAntoine Coeur2020-02-107-16/+16
| | | | | | | | | | | | | | Fix various typos in comments and documentation. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Antoine Coeur <coeur@gmx.fr> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com> Message-Id: <20200207010831.9046-78-philmd@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEsLaszlo Ersek2020-01-171-4/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In commit 4eee0cc7cc0d ("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports", 2019-07-12), the Page Directory Entry setting was regressed (corrupted) when splitting a 2MB page to 512 4KB pages, in the InitPaging() function. Consider the following hunk, displayed with $ git show --function-context --ignore-space-change 4eee0cc7cc0db > // > // If it is 2M page, check IsAddressSplit() > // > if (((*Pd & IA32_PG_PS) != 0) && IsAddressSplit (Address)) { > // > // Based on current page table, create 4KB page table for split area. > // > ASSERT (Address == (*Pd & PHYSICAL_ADDRESS_MASK)); > > Pt = AllocatePageTableMemory (1); > ASSERT (Pt != NULL); > > + *Pd = (UINTN) Pt | IA32_PG_RW | IA32_PG_P; > + > // Split it > - for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++) { > - Pt[PtIndex] = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS); > + for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++, Pt++) { > + *Pt = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS); > } // end for PT > *Pd = (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBUTE_BITS; > } // end if IsAddressSplit > } // end for PD First, the new assignment to the Page Directory Entry (*Pd) is superfluous. That's because (a) we set (*Pd) after the Page Table Entry loop anyway, and (b) here we do not attempt to access the memory starting at "Address" (which is mapped by the original value of the Page Directory Entry). Second, appending "Pt++" to the incrementing expression of the PTE loop is a bug. It causes "Pt" to point *right past* the just-allocated Page Table, once we finish the loop. But the PDE assignment that immediately follows the loop assumes that "Pt" still points to the *start* of the new Page Table. The result is that the originally mapped 2MB page disappears from the processor's view. The PDE now points to a "Page Table" that is filled with garbage. The random entries in that "Page Table" will cause some virtual addresses in the original 2MB area to fault. Other virtual addresses in the same range will no longer have a 1:1 physical mapping, but be scattered over random physical page frames. The second phase of the InitPaging() function ("Go through page table and set several page table entries to absent or execute-disable") already manipulates entries in wrong Page Tables, for such PDEs that got split in the first phase. This issue has been caught as follows: - OVMF is started with 2001 MB of guest RAM. - This places the main SMRAM window at 0x7C10_1000. - The SMRAM management in the SMM Core links this SMRAM window into "mSmmMemoryMap", with a FREE_PAGE_LIST record placed at the start of the area. - At "SMM Ready To Lock" time, PiSmmCpuDxeSmm calls InitPaging(). The first phase (quoted above) decides to split the 2MB page at 0x7C00_0000 into 512 4KB pages, and corrupts the PDE. The new Page Table is allocated at 0x7CE0_D000, but the PDE is set to 0x7CE0_E000 (plus attributes 0x67). - Due to the corrupted PDE, the second phase of InitPaging() already looks up the PTE for Address=0x7C10_1000 in the wrong place. The second phase goes on to mark bogus PTEs as "NX". - PiSmmCpuDxeSmm calls SetMemMapAttributes(). Address 0x7C10_1000 is at the base of the SMRAM window, therefore it happens to be listed in the SMRAM map as an EfiConventionalMemory region. SetMemMapAttributes() calls SmmSetMemoryAttributes() to mark the region as XP. However, GetPageTableEntry() in ConvertMemoryPageAttributes() fails -- address 0x7C10_1000 is no longer mapped by anything! -- and so the attribute setting fails with RETURN_UNSUPPORTED. This error goes unnoticed, as SetMemMapAttributes() ignores the return value of SmmSetMemoryAttributes(). - When SetMemMapAttributes() reaches another entry in the SMRAM map, ConvertMemoryPageAttributes() decides it needs to split a 2MB page, and calls SplitPage(). - SplitPage() calls AllocatePageTableMemory() for the new Page Table, which takes us to InternalAllocMaxAddress() in the SMM Core. - The SMM core attempts to read the FREE_PAGE_LIST record at 0x7C10_1000. Because this virtual address is no longer mapped, the firmware crashes in InternalAllocMaxAddress(), when accessing (Pages->NumberOfPages). Remove the useless assignment to (*Pd) from before the loop. Revert the loop incrementing and the PTE assignment to the known good version. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1789335 Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> Reviewed-by: Ray Ni <ray.ni@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Add missed comments for parameter.Eric Dong2020-01-101-0/+2
| | | | | | | | | | | This issue caused by below change: SHA-1: b948a496150f4ae4f656c0f0ab672608723c80e6 * UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate PROCEDURE_TOKEN buffer REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 Reviewed-by: Ray Ni <ray.ni@intel.com> Acked-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: improve the coding styleEric Dong2020-01-081-9/+9
| | | | | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2434 Current code use below loops to enumerate the CPUs: for (Index = mMaxNumberOfCpus; Index-- > 0;) { it has no issue but not easy for the developers to read the code. Update above code to below style, for (Index = 0; Index < mMaxNumberOfCpus; Index++) { It make the developers easy to read and consistent with other similar cases in this driver. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* Revert "UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue."Eric Dong2020-01-081-8/+8
| | | | | | | | | | This reverts commit 123b720eeb371e0a31eb727bcf59255b584e355f. The commit message for commit 123b720eeb37 is not correct. Cc: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate PROCEDURE_TOKEN bufferEric Dong2020-01-022-87/+109
| | | | | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 Token is new introduced by MM MP Protocol. Current logic allocate Token every time when need to use it. The logic caused SMI latency raised to very high. Update logic to allocate Token buffer at driver's entry point. Later use the token from the allocated token buffer. Only when all the buffer have been used, then need to allocate new buffer. Former change (9caaa79dd7e078ebb4012dde3b3d3a5d451df609) missed PROCEDURE_TOKEN part, this change covers it. Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.Eric Dong2019-12-241-8/+8
| | | | | | | | | | | | The size for the array of mSmmMpSyncData->CpuData[] is 0 ~ mMaxNumberOfCpus -1. But current code may use mSmmMpSyncData->CpuData[mMaxNumberOfCpus]. This patch fixed this issue. Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APsEric Dong2019-12-242-85/+45
| | | | | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2268 In current implementation, when check whether APs called by StartUpAllAPs or StartUpThisAp, it checks the Tokens value used by other APs. Also the AP will update the Token value for itself if its task finished. In this case, the potential race condition issues happens for the tokens. Because of this, system may trig ASSERT during cycling test. This change enhance the code logic, add new attributes for the token to remove the reference for the tokens belongs to other APs. Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every timeEric Dong2019-12-063-4/+81
| | | | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 Token is new introduced by MM MP Protocol. Current logic allocate Token every time when need to use it. The logic caused SMI latency raised to very high. Update logic to allocate Token buffer at driver's entry point. Later use the token from the allocated token buffer. Only when all the buffer have been used, then need to allocate new buffer. Reviewed-by: Ray Ni <ray.ni@intel.com> Signed-off-by: Eric Dong <eric.dong@intel.com> Acked-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg: Update the coding stylesShenglei Zhang2019-12-043-3/+3
| | | | | | | | | | | | In MpLib.c, remove the white space on a new line. In PageTbl.c and PiSmmCpuDxeSmm.h, update the comment style. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg: Fix potential spinLock issue in SmmStartupThisApDamian Nikodem2019-09-201-7/+2
| | | | | | | | | | | | | | | | | | | | | | Due to needs a tackling the deficiency of the AP API, it's necessary to ensure that in non-blocking mode previous AP executed command is finished before starting new one. To remedy above: 1) execute AcquireSpinLock instead AcquireSpinLockOrFail - this will ensure time "window" to eliminate potential race condition between BSP and AP spinLock release in non-blocking mode. This also will eliminate possibility to start executing new AP function before last is finished. 2) remove returns EFI_STATUS - EFI_NOT_READY - in new scenario returned status is not necessary to caller. Signed-off-by: Damian Nikodem <damian.nikodem@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Benjamin You <benjamin.you@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
* UefiCpuPkg/PiSmmCpu: Enable 5L paging only when phy addr line > 48Ray Ni2019-09-132-22/+39
| | | | | | | | | | | | | | | | | | | Today's behavior is to enable 5l paging when CPU supports it (CPUID[7,0].ECX.BIT[16] is set). The patch changes the behavior to enable 5l paging when two conditions are both met: 1. CPU supports it; 2. The max physical address bits is bigger than 48. Because 4-level paging can support to address physical address up to 2^48 - 1, there is no need to enable 5-level paging with max physical address bits <= 48. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Liming Gao <liming.gao@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpu: Restrict access per PcdCpuSmmRestrictedMemoryAccessRay Ni2019-09-044-8/+49
| | | | | | | | | | | | | | | | | | Today's behavior is to always restrict access to non-SMRAM regardless the value of PcdCpuSmmRestrictedMemoryAccess. Because RAS components require to access all non-SMRAM memory, the patch changes the code logic to honor PcdCpuSmmRestrictedMemoryAccess so that only when the PCD is true, the restriction takes affect and page table memory is also protected. Because IA32 build doesn't reference this PCD, such restriction always takes affect in IA32 build. Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpu: Use new PCD PcdCpuSmmRestrictedMemoryAccessRay Ni2019-09-042-22/+34
| | | | | | | | | | | | | | | The patch changes PiSmmCpu driver to consume PCD PcdCpuSmmRestrictedMemoryAccess. Because the behavior controlled by PcdCpuSmmStaticPageTable in original code is not changed after switching to PcdCpuSmmRestrictedMemoryAccess. The functionality is not impacted by this patch. Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic.Dong, Eric2019-08-211-0/+29
| | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040 Supports new logic which test current value before write new value. Only write new value when current value not same as new value. Signed-off-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action.Dong, Eric2019-08-211-42/+62
| | | | | | Signed-off-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to ↵Damian Nikodem2019-08-211-40/+59
| | | | | | | | | | | | | | | | | | | | | handle current page fault Reclaim may free page table pages that are required to handle current page fault. This causes a page leak, and, after sufficent number of specific page fault+reclaim pairs, we run out of reclaimable pages and hit: ASSERT (MinAcc != (UINT64)-1); To remedy, prevent pages essential to handling current page fault: (1) from being considered as reclaim candidates (first reclaim phase) (2) from being freed as part of "branch cleanup" (second reclaim phase) Signed-off-by: Damian Nikodem <damian.nikodem@intel.com> Cc: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Jian J Wang <jian.j.wang@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Fix coding styleShenglei Zhang2019-08-132-6/+6
| | | | | | | | | | | | | 1. Update CPUStatus to CpuStatus in comments to align comments with code. 2. Add "OUT" attribute for "ProcedureArguments" parameter in function header. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Clean up useless ConsoleLogLock spinlock.Eric Dong2019-08-091-2/+0
| | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2060 Remove the useless ConsoleLogLock spinlock. Signed-off-by: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg: Update code to include register definitions from MdePkgNi, Ray2019-08-092-4/+2
| | | | | | | Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Regression-tested-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.Eric Dong2019-08-061-21/+1
| | | | | | | | | | | | This debug message may be called by BSP and APs. It may caused ASSERT when APs call this debug code. In order to avoid system boot assert, Remove this debug message. Signed-off-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Make code consistent with commentsshenglei2019-08-062-3/+3
| | | | | | | | | | | | 1.Remove "out" attribute for " Buffer" parameter in function header. 2.Add "out" attribute for " Token" parameter in function header. 3.Update ProcedureArgument to ProcedureArguments. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Add check for pointer Pml5EntryShenglei Zhang2019-08-051-0/+1
| | | | | | | | | | | | The pointer Pml5Entry, returned from call to function AllocatePageTableMemory, may be null. So add check for it. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Keep function comment and declaration adjacentShenglei Zhang2019-08-021-1/+0
| | | | | | | | | Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* Revert "UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF"Laszlo Ersek2019-07-261-15/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This reverts commit 30f6148546c7092650ab4886fc6d95d5065c3188. Commit 30f6148546c7 causes a build failure, when building for IA32: > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function > 'PerformRemainingTasks': > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: > 'mCpuSmmStaticPageTable' undeclared (first use in this function) > if (mCpuSmmStaticPageTable) { "mCpuSmmStaticPageTable" is an X64-only variable. It is defined in "X64/PageTbl.c", which is not linked into the IA32 binary. We must not reference the variable in such code that is linked into both IA32 and X64 builds, such as "PiSmmCpuDxeSmm.c". We have encountered the same challenge at least once in the past: - https://bugzilla.tianocore.org/show_bug.cgi?id=1593 - commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM", 2019-04-04) The right approach is to declare a new function in "PiSmmCpuDxeSmm.h", and to provide two definitions for the function, one in "Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32 implementation should return a constant value. The X64 implementation should return "mCpuSmmStaticPageTable". (In the example named above, the functions were SaveCr2() and RestoreCr2().) Signed-off-by: Laszlo Ersek <lersek@redhat.com> [lersek@redhat.com: push revert immediately, due to build breakage that would have been easy to catch before submitting the patch]
* UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFFNi, Ray2019-07-261-6/+15
| | | | | | | | | | | | | | | | | | | | | | Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2 * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used updated page fault handler to treat SMM access-out as allowed address when static paging is not used. But that commit is not complete because the page table is still updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM code accesses non-SMRAM memory, page fault is still generated. This patch skips to update page table for non-SMRAM memory and page table itself. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Acked-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP ProtocolEric Dong2019-07-166-25/+1391
| | | | | | | | | | | | REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1937 Add MM Mp Protocol in PiSmmCpuDxeSmm driver. Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpu: ReclaimPages: fix incorrect operator bindingRay Ni2019-07-121-1/+1
| | | | | | | | | Fixes: 4eee0cc7c Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
* UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supportsRay Ni2019-07-125-300/+561
| | | | | | | | | | | | | REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1946 The patch changes SMM environment to use 5 level paging when CPU supports it. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Regression-tested-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com> (cherry picked from commit 7365eb2c8cf1d7112330d09918c0c67e8d0b827a)
* Revert "UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports"Ray Ni2019-07-125-561/+300
| | | | | | | | | | | | | | | | | This reverts commit 7365eb2c8cf1d7112330d09918c0c67e8d0b827a. Commit 7c5010c7f8 MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging technically breaks the EDKII development process documented in https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process and Maintainers.txt in EDKII repo root directory. The voilation is commit 7c5010c7f8 doesn't have a Reviewed-by or Acked-by from MdePkg maintainers. In order to revert 7c5010c7f8, 7365eb2c8 needs to revert first otherwise simply reverting 7c5010c7f8 will cause build break. Signed-off-by: Ray Ni <ray.ni@intel.com>
* UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supportsRay Ni2019-07-105-300/+561
| | | | | | | | | | | | REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1946 The patch changes SMM environment to use 5 level paging when CPU supports it. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Regression-tested-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg/PiSmmCpu: Change variable names and comments to follow SDMRay Ni2019-07-101-57/+57
| | | | | | | | | | | | | | | | | | | Per SDM, for IA-32e 4-KByte paging, there are four layers in the page table structure: 1. PML4 2. Page-Directory-Pointer Table (PDPT) 3. Page-Directory (PD) 4. Page Table (PT) The patch changes the local variable names and comments to use "PML4", "PDPT", "PD", "PT" to better align to terms used in SDM. There is no functionality impact for this change. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Regression-tested-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
* UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read ↵Star Zeng2019-05-141-8/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | (CVE-2018-12182) BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1136 CVE: CVE-2018-12182 Customer met system hang-up during serial port loopback test in OS. It is a corner case happened with one CPU core doing "out dx,al" and another CPU core(s) doing "rep outs dx,byte ptr [rsi]". Detailed code flow is as below. 1. Serial port loopback test in OS. One CPU core: "out dx,al" -> Writing B2h, SMI will happen. Another CPU core(s): "rep outs dx,byte ptr [rsi]". 2. SMI happens to enter SMM. "out dx" (SMM_IO_TYPE_OUT_DX) is saved as I/O instruction type in SMRAM save state for CPU doing "out dx,al". "rep outs dx" (SMM_IO_TYPE_REP_OUTS) is saved as I/O instruction type and rsi is save as I/O Memory Address in SMRAM save state for CPU doing "rep outs dx, byte ptr [rsi]". NOTE: I/O Memory Address (rsi) is a virtual address mapped by OS/Virtual Machine. 3. Some SMM code calls EFI_SMM_CPU_PROTOCOL.ReadSaveState() with EFI_SMM_SAVE_STATE_REGISTER_IO and parse data returned. For example: https://github.com/tianocore/edk2/blob/master/QuarkSocPkg/ QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmSw.c#L76 4. SmmReadSaveState() is executed to read save state for EFI_SMM_SAVE_STATE_REGISTER_IO. - The SmmReadSaveState() function in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c" calls the SmmCpuFeaturesReadSaveStateRegister() function, from the platform's SmmCpuFeaturesLib instance. - If that platform-specific function returns EFI_UNSUPPORTED, then PiSmmCpuDxeSmm falls back to the common function ReadSaveStateRegister(), defined in file "UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c". Current ReadSaveStateRegister() in UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c is trying to copy data from I/O Memory Address for EFI_SMM_SAVE_STATE_IO_TYPE_REP_PREFIX, PF will happen as SMM page table does not know and cover this OS/Virtual Machine virtual address. Same case is for SmmCpuFeaturesReadSaveStateRegister() in platform- specific SmmCpuFeaturesLib instance if it has similar implementation to read save state for EFI_SMM_SAVE_STATE_REGISTER_IO with EFI_SMM_SAVE_STATE_IO_TYPE_REP_PREFIX. Same case is for "ins", 'outs' and 'rep ins'. So to fix the problem, this patch updates the code to only support IN/OUT, but not INS/OUTS/REP INS/REP OUTS for SmmReadSaveState(). Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Star Zeng <star.zeng@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-05-20 09:41:50 +0000