On 2/22/18 6:08 AM, Laszlo Ersek wrote: > On 02/21/18 17:52, Brijesh Singh wrote: >> Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs >> early in PEI phase and clears the C-bit from all MMIO regions (including > s/PEI/DXE/ > > >> Qemu Flash). When SMM is enabled, we build two sets of page tables; first >> page table is used when executing code in non SMM mode (SMM-less-pgtable) >> and second page table is used when we are executing code in SMM mode >> (SMM-pgtable). >> >> During boot time, AmdSevDxe driver clears the C-bit from the >> SMM-less-pgtable. But when SMM is enabled, Qemu Flash services are used >> from SMM mode. >> >> In this patch we explicitly clear the C-bit from Qemu flash MMIO range >> before we probe the flash. When OVMF is built with SMM_REQUIRE then >> call to initialize the flash services happen after the SMM-pgtable is >> created and processor is serving the first SMI. At this time we will >> have access to the SMM-pgtable. > The problem statement is good (including the comment in the code). > > However, I would prefer if we could reflect the full AmdSevDxe logic to > the SMM page tables. In other words, when -- or shortly after -- the SMM > page tables are built, we should clear the C-bit in all those PTEs that > cover known MMIO and as-yet NonExistent memory ranges. We already have a > bunch of "mAddressEncMask" usage in PiSmmCpuDxeSmm. > > Can we investigate this a bit? If it turns out to be impossible, I guess > I might be OK with this patch.
I will investigate this a bit. The reason why I didn't replicated full AmdSevDxe logic is because I thought in SMM world we don't need to do all those MMIO accesses etc but if its not the case then I agree we should implement the full logic here. > > I have more comments: > > >> Cc: Jordan Justen <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Cc: Ard Biesheuvel <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <[email protected]> >> --- >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h | 5 +++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 5 +++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 10 ++++++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 35 >> ++++++++++++++++++++ >> 5 files changed, 56 insertions(+) >> >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf >> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf >> index ba2d3679a46d..d365e27cbe59 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf >> @@ -53,6 +53,7 @@ [LibraryClasses] >> DevicePathLib >> DxeServicesTableLib >> MemoryAllocationLib >> + MemEncryptSevLib >> PcdLib >> SmmServicesTableLib >> UefiBootServicesTableLib >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> index 1f9287b08769..704ed477ba14 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> @@ -189,4 +189,9 @@ VOID >> InstallVirtualAddressChangeHandler ( >> VOID >> ); >> + >> +VOID >> +FvbBeforeFlashProbe ( >> + VOID >> + ); >> #endif > Please drop the "Fvb" prefix; this function is not an FVB protocol member. Will do. > > >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> index 558b395dff4a..b7b9bf1fb8d9 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> @@ -967,6 +967,11 @@ FvbInitialize ( >> UINTN NumOfBlocks; >> RETURN_STATUS PcdStatus; >> >> + // >> + // execute platform specific hooks before probing the flash >> + // >> + FvbBeforeFlashProbe (); >> + >> if (EFI_ERROR (QemuFlashInitialize ())) { >> // >> // Return an error so image will be unloaded >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> index 63b308658e36..7d274c08ad12 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> @@ -155,3 +155,13 @@ InstallVirtualAddressChangeHandler ( >> ); >> ASSERT_EFI_ERROR (Status); >> } >> + >> +VOID >> +FvbBeforeFlashProbe ( >> + VOID >> + ) >> +{ >> + // >> + // Do nothing >> + // >> +} >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> index e0617f2503a2..d97b13f47bf7 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> @@ -17,6 +17,7 @@ >> #include <Library/DebugLib.h> >> #include <Library/PcdLib.h> >> #include <Library/SmmServicesTableLib.h> >> +#include <Library/MemEncryptSevLib.h> >> #include <Protocol/DevicePath.h> >> #include <Protocol/SmmFirmwareVolumeBlock.h> >> >> @@ -67,3 +68,37 @@ InstallVirtualAddressChangeHandler ( >> // Nothing. >> // >> } >> + >> +VOID >> +FvbBeforeFlashProbe ( >> + VOID >> + ) >> +{ >> + >> + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); >> + >> + // >> + // When SEV is enabled, AmdSevDxe runs early in PEI phase and clears the >> C-bit > s/PEI/DXE/ Will do. > > >> + // from the MMIO space (including flash ranges) but the driver runs in >> non SMM >> + // context hence it cleared the flash ranges from non SMM page table. >> + // When SMM is enabled, the flash services are accessed from the SMM mode >> + // hence we explicitly clear the C-bit on flash ranges from SMM page >> table. >> + // >> + if (MemEncryptSevIsEnabled ()) { >> + EFI_STATUS Status; >> + EFI_PHYSICAL_ADDRESS BaseAddress; >> + UINTN FdBlockSize, FdBlockCount; >> + >> + BaseAddress = (EFI_PHYSICAL_ADDRESS) PcdGet32 (PcdOvmfFdBaseAddress); >> + FdBlockSize = PcdGet32 (PcdOvmfFirmwareBlockSize); >> + FdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / FdBlockSize; >> + >> + Status = MemEncryptSevClearPageEncMask ( >> + 0, >> + BaseAddress, >> + EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount), >> + FALSE >> + ); >> + ASSERT_EFI_ERROR (Status); >> + } >> +} >> > I think it would be better to hook this logic into > QemuFlashInitialize(). That function already computes mFlashBase, > mFdBlockSize and mFdBlockCount. Right before the call to > QemuFlashDetected(), we could call BeforeFlashProbe(). The latter could > take the base address, the block size and count as parameters, or just > use the global variables. > Let me see what I can do. > But, again, my preference would be to mirror the AmdSevDxe logic into > (or right after) the SMM page table setup. Perhaps that can be done in > SmmCpuFeaturesInitializeProcessor(), when IsMonarch is TRUE -- this > function is called from SmmInitHandler(), and at that point, the SMM > page tables are already in use. (See above the SmmInitHandler() call > site in "UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm".) Ah, I didn't know this one. I got SMM working with very small patch set hence never looked in UefiCpuPkg for complete understanding of various SmmFeatureLib routines but now I am looking more into it and I think we may able to use SmmCpuFeatureInitializeProcessor() routines. > Thanks, > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
