* [PATCH v2 0/2] Add SMM support when SEV is active @ 2018-02-28 16:14 Brijesh Singh 2018-02-28 16:14 ` [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh 2018-02-28 16:14 ` [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh 0 siblings, 2 replies; 7+ messages in thread From: Brijesh Singh @ 2018-02-28 16:14 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, Paolo Bonzini, Michael Kinney, Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel The series adds the SMM support for the SEV guest. Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> repo: https://github.com/codomania/edk2.git branch: smm-v2 Changes since v1: - add more comments to explain why we are not clearing the C-bit from relocated SMM Saved area. - restore the C-bit of initial SMM Saved Area after SMBASE is relocated. - Drop "Fvb" prefix from BeforeFlashProbe() and call this from Qemu flash initialization. Brijesh Singh (2): OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 +++ OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 + OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h | 7 ++++ OvmfPkg/AmdSevDxe/AmdSevDxe.c | 35 ++++++++++++++++++++ OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 ++++++++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 6 ++++ 9 files changed, 120 insertions(+) -- 2.14.3 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-28 16:14 [PATCH v2 0/2] Add SMM support when SEV is active Brijesh Singh @ 2018-02-28 16:14 ` Brijesh Singh 2018-02-28 19:06 ` Laszlo Ersek 2018-02-28 16:14 ` [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh 1 sibling, 1 reply; 7+ messages in thread From: Brijesh Singh @ 2018-02-28 16:14 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, Paolo Bonzini, Michael Kinney, Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by both guest and hypervisor. Since the data need to be accessed by both hence we must map the SMMSaved State area as unencrypted (i.e C-bit cleared). This patch clears the SavedStateArea address before SMBASE relocation. Currently, we do not clear the SavedStateArea address after SMBASE is relocated due to the following reasons: 1) Guest BIOS never access the relocated SavedStateArea. 2) The C-bit works on page-aligned address, but the SavedStateArea address is not a page-aligned. Theoretically, we could roundup the address and clear the C-bit of aligned address but looking carefully we found that some portion of the page contains code -- which will causes a bigger issue for the SEV guest. When SEV is enabled, all the code must be encrypted otherwise hardware will cause trap. Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 +++ OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 + OvmfPkg/AmdSevDxe/AmdSevDxe.c | 35 ++++++++++++++++++++ OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 ++++++++++++ 4 files changed, 61 insertions(+) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf index 41635a57a454..162ed98a2fbe 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf @@ -29,6 +29,7 @@ [Packages] MdePkg/MdePkg.dec MdeModulePkg/MdeModulePkg.dec OvmfPkg/OvmfPkg.dec + UefiCpuPkg/UefiCpuPkg.dec [LibraryClasses] BaseLib @@ -41,3 +42,6 @@ [LibraryClasses] [Depex] TRUE + +[FeaturePcd] + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf index 31edf3a9c1fd..ba564abb787b 100644 --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf @@ -36,3 +36,4 @@ [LibraryClasses] PcdLib DebugLib SmmServicesTableLib + MemEncryptSevLib diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index e472096320ea..5803e8655049 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -25,6 +25,8 @@ #include <Library/UefiBootServicesTableLib.h> #include <Library/DxeServicesTableLib.h> #include <Library/MemEncryptSevLib.h> +#include <Register/SmramSaveStateMap.h> +#include <Register/QemuSmramSaveStateMap.h> EFI_STATUS EFIAPI @@ -71,5 +73,38 @@ AmdSevDxeEntryPoint ( FreePool (AllDescMap); } + // + // When SMM is enabled, clear the C-bit from SMM Saved State Area + // + // NOTES: The SavedStateArea address cleared here is before SMBASE + // relocation. Currently, we do not clear the SavedStateArea address after + // SMBASE is relocated due to the following reasons: + // + // 1) Guest BIOS never access the relocated SavedStateArea. + // + // 2) The C-bit works on page-aligned address, but the SavedStateArea + // address is not a page-aligned. Theoretically, we could roundup the address + // and clear the C-bit of aligned address but looking carefully we found + // that some portion of the page contains code -- which will causes a bigger + // issues for SEV guest. When SEV is enabled, all the code must be encrypted + // otherwise hardware will cause trap. + // + // We restore the C-bit for this SMM Saved State Area after SMBASE relocation + // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c). + // + if (FeaturePcdGet (PcdSmmSmramRequire)) { + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; + + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; + + Status = MemEncryptSevClearPageEncMask ( + 0, + SmmSavedStateAreaAddress, + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), + FALSE + ); + ASSERT_EFI_ERROR (Status); + } + return EFI_SUCCESS; } diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c index a307f64c9c61..946294701c62 100644 --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c @@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #include <Library/MemoryAllocationLib.h> #include <Library/SmmServicesTableLib.h> #include <Library/DebugLib.h> +#include <Library/MemEncryptSevLib.h> #include <Register/QemuSmramSaveStateMap.h> // @@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete ( VOID ) { + EFI_STATUS Status; + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; + + // + // When SEV is enabled, the SMM SavedState is mapped with C=0 + // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we + // remap the address with C=1. + // + if (!MemEncryptSevIsEnabled ()) { + return; + } + + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; + Status = MemEncryptSevSetPageEncMask ( + 0, + SmmSavedStateAreaAddress, + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), + FALSE + ); + ASSERT_EFI_ERROR (Status); } /** -- 2.14.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-28 16:14 ` [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh @ 2018-02-28 19:06 ` Laszlo Ersek 2018-02-28 19:23 ` Brijesh Singh 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2018-02-28 19:06 UTC (permalink / raw) To: Brijesh Singh, edk2-devel Cc: Tom Lendacky, Paolo Bonzini, Michael Kinney, Jordan Justen, Ard Biesheuvel Hi Brijesh, On 02/28/18 17:14, Brijesh Singh wrote: > When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE + > SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by > both guest and hypervisor. Since the data need to be accessed by both > hence we must map the SMMSaved State area as unencrypted (i.e C-bit > cleared). > > This patch clears the SavedStateArea address before SMBASE relocation. > Currently, we do not clear the SavedStateArea address after SMBASE is > relocated due to the following reasons: > > 1) Guest BIOS never access the relocated SavedStateArea. > > 2) The C-bit works on page-aligned address, but the SavedStateArea > address is not a page-aligned. Theoretically, we could roundup the address > and clear the C-bit of aligned address but looking carefully we found > that some portion of the page contains code -- which will causes a bigger > issue for the SEV guest. When SEV is enabled, all the code must be > encrypted otherwise hardware will cause trap. > > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 +++ > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 + > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 35 ++++++++++++++++++++ > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 ++++++++++++ > 4 files changed, 61 insertions(+) I've been staring at this patch for ~2 hours now (I've also read your other email). I like this approach (and the comments / commit message), but IMO an important detail is missing. I started writing up my notes, but the list got very long. Is it OK with you if I send my ideas as a patch set (replacing just this patch)? I think I'd like to turn this patch into 3-4 patches. Thanks! Laszlo > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > index 41635a57a454..162ed98a2fbe 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > @@ -29,6 +29,7 @@ [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > OvmfPkg/OvmfPkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > > [LibraryClasses] > BaseLib > @@ -41,3 +42,6 @@ [LibraryClasses] > > [Depex] > TRUE > + > +[FeaturePcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > index 31edf3a9c1fd..ba564abb787b 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > @@ -36,3 +36,4 @@ [LibraryClasses] > PcdLib > DebugLib > SmmServicesTableLib > + MemEncryptSevLib > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index e472096320ea..5803e8655049 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > @@ -25,6 +25,8 @@ > #include <Library/UefiBootServicesTableLib.h> > #include <Library/DxeServicesTableLib.h> > #include <Library/MemEncryptSevLib.h> > +#include <Register/SmramSaveStateMap.h> > +#include <Register/QemuSmramSaveStateMap.h> > > EFI_STATUS > EFIAPI > @@ -71,5 +73,38 @@ AmdSevDxeEntryPoint ( > FreePool (AllDescMap); > } > > + // > + // When SMM is enabled, clear the C-bit from SMM Saved State Area > + // > + // NOTES: The SavedStateArea address cleared here is before SMBASE > + // relocation. Currently, we do not clear the SavedStateArea address after > + // SMBASE is relocated due to the following reasons: > + // > + // 1) Guest BIOS never access the relocated SavedStateArea. > + // > + // 2) The C-bit works on page-aligned address, but the SavedStateArea > + // address is not a page-aligned. Theoretically, we could roundup the address > + // and clear the C-bit of aligned address but looking carefully we found > + // that some portion of the page contains code -- which will causes a bigger > + // issues for SEV guest. When SEV is enabled, all the code must be encrypted > + // otherwise hardware will cause trap. > + // > + // We restore the C-bit for this SMM Saved State Area after SMBASE relocation > + // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c). > + // > + if (FeaturePcdGet (PcdSmmSmramRequire)) { > + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; > + > + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; > + > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + SmmSavedStateAreaAddress, > + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), > + FALSE > + ); > + ASSERT_EFI_ERROR (Status); > + } > + > return EFI_SUCCESS; > } > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index a307f64c9c61..946294701c62 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > @@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #include <Library/MemoryAllocationLib.h> > #include <Library/SmmServicesTableLib.h> > #include <Library/DebugLib.h> > +#include <Library/MemEncryptSevLib.h> > #include <Register/QemuSmramSaveStateMap.h> > > // > @@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete ( > VOID > ) > { > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; > + > + // > + // When SEV is enabled, the SMM SavedState is mapped with C=0 > + // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we > + // remap the address with C=1. > + // > + if (!MemEncryptSevIsEnabled ()) { > + return; > + } > + > + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; > + Status = MemEncryptSevSetPageEncMask ( > + 0, > + SmmSavedStateAreaAddress, > + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), > + FALSE > + ); > + ASSERT_EFI_ERROR (Status); > } > > /** > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-28 19:06 ` Laszlo Ersek @ 2018-02-28 19:23 ` Brijesh Singh 0 siblings, 0 replies; 7+ messages in thread From: Brijesh Singh @ 2018-02-28 19:23 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Paolo Bonzini, Michael Kinney, Jordan Justen, Ard Biesheuvel Hi Laszlo, On 2/28/18 1:06 PM, Laszlo Ersek wrote: > Hi Brijesh, > > On 02/28/18 17:14, Brijesh Singh wrote: >> When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE + >> SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by >> both guest and hypervisor. Since the data need to be accessed by both >> hence we must map the SMMSaved State area as unencrypted (i.e C-bit >> cleared). >> >> This patch clears the SavedStateArea address before SMBASE relocation. >> Currently, we do not clear the SavedStateArea address after SMBASE is >> relocated due to the following reasons: >> >> 1) Guest BIOS never access the relocated SavedStateArea. >> >> 2) The C-bit works on page-aligned address, but the SavedStateArea >> address is not a page-aligned. Theoretically, we could roundup the address >> and clear the C-bit of aligned address but looking carefully we found >> that some portion of the page contains code -- which will causes a bigger >> issue for the SEV guest. When SEV is enabled, all the code must be >> encrypted otherwise hardware will cause trap. >> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 +++ >> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 + >> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 35 ++++++++++++++++++++ >> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 ++++++++++++ >> 4 files changed, 61 insertions(+) > I've been staring at this patch for ~2 hours now (I've also read your > other email). I like this approach (and the comments / commit message), > but IMO an important detail is missing. > > I started writing up my notes, but the list got very long. Is it OK with > you if I send my ideas as a patch set (replacing just this patch)? I > think I'd like to turn this patch into 3-4 patches. Yes, patches are appreciated. thanks. > > Thanks! > Laszlo > > >> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> index 41635a57a454..162ed98a2fbe 100644 >> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> @@ -29,6 +29,7 @@ [Packages] >> MdePkg/MdePkg.dec >> MdeModulePkg/MdeModulePkg.dec >> OvmfPkg/OvmfPkg.dec >> + UefiCpuPkg/UefiCpuPkg.dec >> >> [LibraryClasses] >> BaseLib >> @@ -41,3 +42,6 @@ [LibraryClasses] >> >> [Depex] >> TRUE >> + >> +[FeaturePcd] >> + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> index 31edf3a9c1fd..ba564abb787b 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> @@ -36,3 +36,4 @@ [LibraryClasses] >> PcdLib >> DebugLib >> SmmServicesTableLib >> + MemEncryptSevLib >> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> index e472096320ea..5803e8655049 100644 >> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> @@ -25,6 +25,8 @@ >> #include <Library/UefiBootServicesTableLib.h> >> #include <Library/DxeServicesTableLib.h> >> #include <Library/MemEncryptSevLib.h> >> +#include <Register/SmramSaveStateMap.h> >> +#include <Register/QemuSmramSaveStateMap.h> >> >> EFI_STATUS >> EFIAPI >> @@ -71,5 +73,38 @@ AmdSevDxeEntryPoint ( >> FreePool (AllDescMap); >> } >> >> + // >> + // When SMM is enabled, clear the C-bit from SMM Saved State Area >> + // >> + // NOTES: The SavedStateArea address cleared here is before SMBASE >> + // relocation. Currently, we do not clear the SavedStateArea address after >> + // SMBASE is relocated due to the following reasons: >> + // >> + // 1) Guest BIOS never access the relocated SavedStateArea. >> + // >> + // 2) The C-bit works on page-aligned address, but the SavedStateArea >> + // address is not a page-aligned. Theoretically, we could roundup the address >> + // and clear the C-bit of aligned address but looking carefully we found >> + // that some portion of the page contains code -- which will causes a bigger >> + // issues for SEV guest. When SEV is enabled, all the code must be encrypted >> + // otherwise hardware will cause trap. >> + // >> + // We restore the C-bit for this SMM Saved State Area after SMBASE relocation >> + // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c). >> + // >> + if (FeaturePcdGet (PcdSmmSmramRequire)) { >> + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; >> + >> + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; >> + >> + Status = MemEncryptSevClearPageEncMask ( >> + 0, >> + SmmSavedStateAreaAddress, >> + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), >> + FALSE >> + ); >> + ASSERT_EFI_ERROR (Status); >> + } >> + >> return EFI_SUCCESS; >> } >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> index a307f64c9c61..946294701c62 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> @@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> #include <Library/MemoryAllocationLib.h> >> #include <Library/SmmServicesTableLib.h> >> #include <Library/DebugLib.h> >> +#include <Library/MemEncryptSevLib.h> >> #include <Register/QemuSmramSaveStateMap.h> >> >> // >> @@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete ( >> VOID >> ) >> { >> + EFI_STATUS Status; >> + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; >> + >> + // >> + // When SEV is enabled, the SMM SavedState is mapped with C=0 >> + // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we >> + // remap the address with C=1. >> + // >> + if (!MemEncryptSevIsEnabled ()) { >> + return; >> + } >> + >> + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; >> + Status = MemEncryptSevSetPageEncMask ( >> + 0, >> + SmmSavedStateAreaAddress, >> + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), >> + FALSE >> + ); >> + ASSERT_EFI_ERROR (Status); >> } >> >> /** >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-28 16:14 [PATCH v2 0/2] Add SMM support when SEV is active Brijesh Singh 2018-02-28 16:14 ` [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh @ 2018-02-28 16:14 ` Brijesh Singh 2018-02-28 19:41 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Brijesh Singh @ 2018-02-28 16:14 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, Paolo Bonzini, Michael Kinney, Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs early in DXE phase and clears the C-bit from all MMIO regions (including 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. Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h | 7 +++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 6 ++++ 5 files changed, 59 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/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h index 8d83dca7a52c..6c4099c140e8 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h @@ -88,5 +88,12 @@ QemuFlashConvertPointers ( VOID ); +VOID +BeforeFlashProbe ( + EFI_PHYSICAL_ADDRESS BaseAddress, + UINTN FdBlockSize, + UINTN FdBlockCount + ); + #endif diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c index 63b308658e36..a4614de3c901 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c @@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler ( ); ASSERT_EFI_ERROR (Status); } + +VOID +BeforeFlashProbe ( + EFI_PHYSICAL_ADDRESS BaseAddress, + UINTN FdBlockSize, + UINTN FdBlockCount + ) +{ + // + // Do nothing + // +} diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c index e0617f2503a2..a6cad5af223b 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,35 @@ InstallVirtualAddressChangeHandler ( // Nothing. // } + +VOID +BeforeFlashProbe ( + EFI_PHYSICAL_ADDRESS BaseAddress, + UINTN FdBlockSize, + UINTN FdBlockCount + ) +{ + EFI_STATUS Status; + + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); + + if (!MemEncryptSevIsEnabled()) { + return; + } + + // + // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit + // 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. + // + + Status = MemEncryptSevClearPageEncMask ( + 0, + BaseAddress, + EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount), + FALSE + ); + ASSERT_EFI_ERROR (Status); +} diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c index 5677b5ee119c..f63e11723415 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c @@ -244,6 +244,12 @@ QemuFlashInitialize ( ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0); mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize; + // + // execute platform specific hooks before probing the flash + // + BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase, + mFdBlockSize, mFdBlockCount); + if (!QemuFlashDetected ()) { ASSERT (!FeaturePcdGet (PcdSmmSmramRequire)); return EFI_WRITE_PROTECTED; -- 2.14.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-28 16:14 ` [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh @ 2018-02-28 19:41 ` Laszlo Ersek 2018-03-01 15:02 ` Brijesh Singh 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2018-02-28 19:41 UTC (permalink / raw) To: Brijesh Singh, edk2-devel Cc: Tom Lendacky, Paolo Bonzini, Michael Kinney, Jordan Justen, Ard Biesheuvel On 02/28/18 17:14, Brijesh Singh wrote: > Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs > early in DXE phase and clears the C-bit from all MMIO regions (including > Qemu Flash). (1) This appears incorrect / inexact; AmdSevDxe is dispatched from APRIORI DXE before the flash driver is dispatched, and the MMIO GCD entry is only added by the flash driver. So in this case, AmdSevDxe clears the C-bit on a NonExistent entry that will later be split and accommodate the flash MMIO range. > 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. (2) Please replace "is serving" with "has served". > > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h | 7 +++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 6 ++++ > 5 files changed, 59 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/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > index 8d83dca7a52c..6c4099c140e8 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > @@ -88,5 +88,12 @@ QemuFlashConvertPointers ( > VOID > ); > > +VOID > +BeforeFlashProbe ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN FdBlockSize, > + UINTN FdBlockCount > + ); > + > #endif (3) Sorry that I'm again requesting a name change for this function. Can we call it QemuFlashBeforeProbe()? To be consistent with the other function names in this header file. (4) Please add "IN" decorators (also to the function definitions). > > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > index 63b308658e36..a4614de3c901 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > @@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler ( > ); > ASSERT_EFI_ERROR (Status); > } > + > +VOID > +BeforeFlashProbe ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN FdBlockSize, > + UINTN FdBlockCount > + ) > +{ > + // > + // Do nothing > + // > +} (5) This function definition should go into the existent file "QemuFlashDxe.c". > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > index e0617f2503a2..a6cad5af223b 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,35 @@ InstallVirtualAddressChangeHandler ( > // Nothing. > // > } > + > +VOID > +BeforeFlashProbe ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN FdBlockSize, > + UINTN FdBlockCount > + ) > +{ > + EFI_STATUS Status; > + > + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > + > + if (!MemEncryptSevIsEnabled()) { > + return; > + } > + > + // > + // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit > + // from the MMIO space (including flash ranges) but the driver runs in non SMM (6) Please update the comment according to (1). > + // 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. > + // > + > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + BaseAddress, > + EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount), > + FALSE > + ); (7) The closing paren is not indented correctly, it should be aligned with the arguments. > + ASSERT_EFI_ERROR (Status); > +} (8) This function definition should go into a new file called "QemuFlashSmm.c" -- please make sure you add a license block at the top, and use CRLF line endings --, and the new file should be added to "FvbServicesSmm.inf". > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > index 5677b5ee119c..f63e11723415 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > @@ -244,6 +244,12 @@ QemuFlashInitialize ( > ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0); > mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize; > > + // > + // execute platform specific hooks before probing the flash > + // (9) Please replace "platform" with "module type". > + BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase, > + mFdBlockSize, mFdBlockCount); > + (10) The indentation is not idiomatic. > if (!QemuFlashDetected ()) { > ASSERT (!FeaturePcdGet (PcdSmmSmramRequire)); > return EFI_WRITE_PROTECTED; > I think this patch is good, just a few warts left to clean up. Thanks! Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-28 19:41 ` Laszlo Ersek @ 2018-03-01 15:02 ` Brijesh Singh 0 siblings, 0 replies; 7+ messages in thread From: Brijesh Singh @ 2018-03-01 15:02 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Paolo Bonzini, Michael Kinney, Jordan Justen, Ard Biesheuvel On 02/28/2018 01:41 PM, Laszlo Ersek wrote: > On 02/28/18 17:14, Brijesh Singh wrote: >> Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs >> early in DXE phase and clears the C-bit from all MMIO regions (including >> Qemu Flash). > > (1) This appears incorrect / inexact; AmdSevDxe is dispatched from > APRIORI DXE before the flash driver is dispatched, and the MMIO GCD > entry is only added by the flash driver. So in this case, AmdSevDxe > clears the C-bit on a NonExistent entry that will later be split and > accommodate the flash MMIO range. > Okay, I will update it. >> 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. > > (2) Please replace "is serving" with "has served". > Will do >> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h | 7 +++++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 6 ++++ >> 5 files changed, 59 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/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h >> index 8d83dca7a52c..6c4099c140e8 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h >> @@ -88,5 +88,12 @@ QemuFlashConvertPointers ( >> VOID >> ); >> >> +VOID >> +BeforeFlashProbe ( >> + EFI_PHYSICAL_ADDRESS BaseAddress, >> + UINTN FdBlockSize, >> + UINTN FdBlockCount >> + ); >> + >> #endif > > (3) Sorry that I'm again requesting a name change for this function. Can > we call it QemuFlashBeforeProbe()? To be consistent with the other > function names in this header file. > > (4) Please add "IN" decorators (also to the function definitions). > Will do >> >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> index 63b308658e36..a4614de3c901 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> @@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler ( >> ); >> ASSERT_EFI_ERROR (Status); >> } >> + >> +VOID >> +BeforeFlashProbe ( >> + EFI_PHYSICAL_ADDRESS BaseAddress, >> + UINTN FdBlockSize, >> + UINTN FdBlockCount >> + ) >> +{ >> + // >> + // Do nothing >> + // >> +} > > (5) This function definition should go into the existent file > "QemuFlashDxe.c". I will look into it. > >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> index e0617f2503a2..a6cad5af223b 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,35 @@ InstallVirtualAddressChangeHandler ( >> // Nothing. >> // >> } >> + >> +VOID >> +BeforeFlashProbe ( >> + EFI_PHYSICAL_ADDRESS BaseAddress, >> + UINTN FdBlockSize, >> + UINTN FdBlockCount >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); >> + >> + if (!MemEncryptSevIsEnabled()) { >> + return; >> + } >> + >> + // >> + // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit >> + // from the MMIO space (including flash ranges) but the driver runs in non SMM > > (6) Please update the comment according to (1). > Will do >> + // 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. >> + // >> + >> + Status = MemEncryptSevClearPageEncMask ( >> + 0, >> + BaseAddress, >> + EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount), >> + FALSE >> + ); > > (7) The closing paren is not indented correctly, it should be aligned > with the arguments. > Will fix it >> + ASSERT_EFI_ERROR (Status); >> +} > > (8) This function definition should go into a new file called > "QemuFlashSmm.c" -- please make sure you add a license block at the top, > and use CRLF line endings --, and the new file should be added to > "FvbServicesSmm.inf". > Will do >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> index 5677b5ee119c..f63e11723415 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> @@ -244,6 +244,12 @@ QemuFlashInitialize ( >> ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0); >> mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize; >> >> + // >> + // execute platform specific hooks before probing the flash >> + // > > (9) Please replace "platform" with "module type". > Will do >> + BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase, >> + mFdBlockSize, mFdBlockCount); >> + > > (10) The indentation is not idiomatic. > >> if (!QemuFlashDetected ()) { >> ASSERT (!FeaturePcdGet (PcdSmmSmramRequire)); >> return EFI_WRITE_PROTECTED; >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-01 14:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-28 16:14 [PATCH v2 0/2] Add SMM support when SEV is active Brijesh Singh 2018-02-28 16:14 ` [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh 2018-02-28 19:06 ` Laszlo Ersek 2018-02-28 19:23 ` Brijesh Singh 2018-02-28 16:14 ` [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh 2018-02-28 19:41 ` Laszlo Ersek 2018-03-01 15:02 ` Brijesh Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox