* [PATCH 0/2] Add SMM support when SEV is active @ 2018-02-21 16:52 Brijesh Singh 2018-02-21 16:52 ` [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh 2018-02-21 16:52 ` [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh 0 siblings, 2 replies; 11+ messages in thread From: Brijesh Singh @ 2018-02-21 16:52 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel The series adds the SMM support for the SEV guest. Brijesh Singh (2): OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 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-v1 OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 +++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h | 5 +++ OvmfPkg/AmdSevDxe/AmdSevDxe.c | 19 +++++++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 5 +++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 10 ++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 35 ++++++++++++++++++++ 7 files changed, 79 insertions(+) -- 2.14.3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-21 16:52 [PATCH 0/2] Add SMM support when SEV is active Brijesh Singh @ 2018-02-21 16:52 ` Brijesh Singh 2018-02-22 11:20 ` Laszlo Ersek 2018-02-21 16:52 ` [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh 1 sibling, 1 reply; 11+ messages in thread From: Brijesh Singh @ 2018-02-21 16:52 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, 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). 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/AmdSevDxe/AmdSevDxe.c | 19 +++++++++++++++++++ 2 files changed, 23 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/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index e472096320ea..5ec727456526 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,22 @@ AmdSevDxeEntryPoint ( FreePool (AllDescMap); } + // + // When SMM is enabled, clear the C-bit from SMM Saved State Area + // + 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; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-21 16:52 ` [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh @ 2018-02-22 11:20 ` Laszlo Ersek 2018-02-22 11:21 ` Laszlo Ersek 2018-02-27 12:17 ` Brijesh Singh 0 siblings, 2 replies; 11+ messages in thread From: Laszlo Ersek @ 2018-02-22 11:20 UTC (permalink / raw) To: Brijesh Singh, edk2-devel Cc: Tom Lendacky, Jordan Justen, Ard Biesheuvel, Paolo Bonzini, Michael Kinney Hi Brijesh! (adding Paolo and Mike; more comments below) On 02/21/18 17:52, 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). > > 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/AmdSevDxe/AmdSevDxe.c | 19 +++++++++++++++++++ > 2 files changed, 23 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/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index e472096320ea..5ec727456526 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,22 @@ AmdSevDxeEntryPoint ( > FreePool (AllDescMap); > } > > + // > + // When SMM is enabled, clear the C-bit from SMM Saved State Area > + // > + 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; > } > First, this SMBASE address is only correct before SMBASE relocation. - What guarantees that AmdSevDxe is dispatched, and the new code is executed, before PiSmmCpuDxeSmm performs the SMBASE relocation? - Where/when do we clear encryption for the state map that is used after SMBASE relocation? If we don't do that at all, then why do things work? (I see a whole lot of "mAddressEncMask" in PiSmmCpuDxeSmm; some help would be appreciated.) Second, after SMBASE relocation, when/where do we restore the C bit on the original (default) SMBASE? I think we should do that, otherwise we'll have an info leak there. Third, why is it OK to pass Flush=FALSE? The documentation says, "mostly TRUE except MMIO addresses". The default SMBASE points into memory, not MMIO; there could be normal data there. PiSmmCpuDxeSmm backs up the area before SMBASE relocation, and restores it after. See SmmRelocateBases() in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c": // // Backup original contents at address 0x38000 // CopyMem (BakBuf, U8Ptr, sizeof (BakBuf)); CopyMem (&BakBuf2, CpuStatePtr, sizeof (BakBuf2)); I wonder if we should introduce new SmmCpuFeaturesLib APIs, for managing memory encryption settings around SMBASE relocation. - PiSmmCpuDxeSemm could call these APIs in "strategic places". - The SmmCpuFeaturesLib instances in UefiCpuPkg and QuarkSocPkg would do nothing. - The instace under OvmfPkg would handle the C-bit, dependent on SEV presence. The lib class already has a function called SmmCpuFeaturesSmmRelocationComplete(); we might be able to use that (too). Thanks, Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-22 11:20 ` Laszlo Ersek @ 2018-02-22 11:21 ` Laszlo Ersek 2018-02-27 12:17 ` Brijesh Singh 1 sibling, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2018-02-22 11:21 UTC (permalink / raw) To: Brijesh Singh, edk2-devel Cc: Tom Lendacky, Jordan Justen, Ard Biesheuvel, Paolo Bonzini, Michael Kinney On 02/22/18 12:20, Laszlo Ersek wrote: > Hi Brijesh! > > (adding Paolo and Mike; more comments below) > > On 02/21/18 17:52, 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). >> >> 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/AmdSevDxe/AmdSevDxe.c | 19 +++++++++++++++++++ >> 2 files changed, 23 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/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> index e472096320ea..5ec727456526 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,22 @@ AmdSevDxeEntryPoint ( >> FreePool (AllDescMap); >> } >> >> + // >> + // When SMM is enabled, clear the C-bit from SMM Saved State Area >> + // >> + 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; >> } >> > > First, this SMBASE address is only correct before SMBASE relocation. > > - What guarantees that AmdSevDxe is dispatched, and the new code is > executed, before PiSmmCpuDxeSmm performs the SMBASE relocation? Apologies, I forgot (and missed) that AmdSevDxe is listed in APRIORI DXE. So consider this part answered. Thanks! Laszlo > > - Where/when do we clear encryption for the state map that is used after > SMBASE relocation? If we don't do that at all, then why do things > work? (I see a whole lot of "mAddressEncMask" in PiSmmCpuDxeSmm; some > help would be appreciated.) > > Second, after SMBASE relocation, when/where do we restore the C bit on > the original (default) SMBASE? I think we should do that, otherwise > we'll have an info leak there. > > Third, why is it OK to pass Flush=FALSE? The documentation says, "mostly > TRUE except MMIO addresses". The default SMBASE points into memory, not > MMIO; there could be normal data there. PiSmmCpuDxeSmm backs up the area > before SMBASE relocation, and restores it after. See SmmRelocateBases() > in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c": > > // > // Backup original contents at address 0x38000 > // > CopyMem (BakBuf, U8Ptr, sizeof (BakBuf)); > CopyMem (&BakBuf2, CpuStatePtr, sizeof (BakBuf2)); > > I wonder if we should introduce new SmmCpuFeaturesLib APIs, for managing > memory encryption settings around SMBASE relocation. > - PiSmmCpuDxeSemm could call these APIs in "strategic places". > - The SmmCpuFeaturesLib instances in UefiCpuPkg and QuarkSocPkg would do > nothing. > - The instace under OvmfPkg would handle the C-bit, dependent on SEV > presence. > > The lib class already has a function called > SmmCpuFeaturesSmmRelocationComplete(); we might be able to use that (too). > > Thanks, > Laszlo > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State 2018-02-22 11:20 ` Laszlo Ersek 2018-02-22 11:21 ` Laszlo Ersek @ 2018-02-27 12:17 ` Brijesh Singh 1 sibling, 0 replies; 11+ messages in thread From: Brijesh Singh @ 2018-02-27 12:17 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Jordan Justen, Ard Biesheuvel, Paolo Bonzini, Michael Kinney Hi Laszlo, Apologies for late response. I needed to do some investigation on SMM before responding to you. On 2/22/18 5:20 AM, Laszlo Ersek wrote: > Hi Brijesh! > > (adding Paolo and Mike; more comments below) > > On 02/21/18 17:52, 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). >> >> 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/AmdSevDxe/AmdSevDxe.c | 19 +++++++++++++++++++ >> 2 files changed, 23 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/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> index e472096320ea..5ec727456526 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,22 @@ AmdSevDxeEntryPoint ( >> FreePool (AllDescMap); >> } >> >> + // >> + // When SMM is enabled, clear the C-bit from SMM Saved State Area >> + // >> + 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; >> } >> > First, this SMBASE address is only correct before SMBASE relocation. > > - What guarantees that AmdSevDxe is dispatched, and the new code is > executed, before PiSmmCpuDxeSmm performs the SMBASE relocation? > > - Where/when do we clear encryption for the state map that is used after > SMBASE relocation? If we don't do that at all, then why do things > work? (I see a whole lot of "mAddressEncMask" in PiSmmCpuDxeSmm; some > help would be appreciated.) mAddressEncMask in PiSmmCpuDxeSmm basically ensure that newly created pagetables have the C-bit set for all the pages -- its very similar to what we do when creating non SMM page table. By default we consider all the pages encrypted, if we do not apply the mAddressEncMask to newly created page then we will fail to execute the code because SEV hw needs code to be encrypted. Currently we do not clear the C-bit of relocated SMBASE saved state area. After you asked this question, I did a bit investigation to see how I am booting the SMM enabled SEV guest without clearing the C-bit of relocated SMBASE saved area. The SMBASE is mapped with C=1 in guest, before entering in SMM mode HV saves some values in SMMSaved area -- this is done using HV key, guest BIOS never reads or writes to SMMSaved area hence we are getting lucky. If guest BIOS ever writes or read the value from relocated SMMSaved area then it will get garbage. We should consider clearing the relocated SMMSavedArea but there are two questions: 1) Where/When we should clear the C-bit of relocated SMMSavedArea ? I was looking at multiple function provided by SmmFeatureLib but could not find a right place for it. Most of time the SMMfeatureLib functions are called in non-SMM context and we do not have access to SMM pagetable to clear the C-bit of SMMSavedArea. How about if we introduce a new function in SmmfeatureLib which gets called just after core creates a new SMM page table ? 2) The C-bit works on page-aligned boundary but SmmSaveArea offset does not start with page-aligned boundary. We could still go ahead and clear the full page -- we may leak some information to HV in that case. But the real issue is looking carefully I see that some portion of page may contain code and hence clearing the full page will cause a runtime issues whenever that code gets executed. (I hacked EDKII to do this and could see that if full page is cleared then sometime later we get crash because someone was trying to execute a code which was mapped with C=0). One option which I was thinking is: since access to relocated SMMSavedState is controlled via SmmFeatureLib hence we could simply map the page with C=0 as we enter in the function and restore the C-bit on exit. This will ensure that any access happening to SmmSavedArea is done with C=0. I have also tried this and it seems to work fine. I was not able to find any code in EDKII which trigger a code path which requires access to SmmSavedArea hence I hacked something to test the flow. I think this is clean approach and does not require any changes in EDKII core and should work just fine. > Second, after SMBASE relocation, when/where do we restore the C bit on > the original (default) SMBASE? I think we should do that, otherwise > we'll have an info leak there. Good point, I will see where we can restore the C-bit of original SMBASE. > > Third, why is it OK to pass Flush=FALSE? The documentation says, "mostly > TRUE except MMIO addresses". The default SMBASE points into memory, not > MMIO; there could be normal data there. PiSmmCpuDxeSmm backs up the area > before SMBASE relocation, and restores it after. See SmmRelocateBases() > in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c": Very good point, the flush should be TRUE. I think its my copy/paste. > // > // Backup original contents at address 0x38000 > // > CopyMem (BakBuf, U8Ptr, sizeof (BakBuf)); > CopyMem (&BakBuf2, CpuStatePtr, sizeof (BakBuf2)); > > I wonder if we should introduce new SmmCpuFeaturesLib APIs, for managing > memory encryption settings around SMBASE relocation. See my above response, If you think its acceptable approach then I don't see us needing any changes in SmmFeatureLib. > - PiSmmCpuDxeSemm could call these APIs in "strategic places". > - The SmmCpuFeaturesLib instances in UefiCpuPkg and QuarkSocPkg would do > nothing. > - The instace under OvmfPkg would handle the C-bit, dependent on SEV > presence. > > The lib class already has a function called > SmmCpuFeaturesSmmRelocationComplete(); we might be able to use that (too). > > Thanks, > Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-21 16:52 [PATCH 0/2] Add SMM support when SEV is active Brijesh Singh 2018-02-21 16:52 ` [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh @ 2018-02-21 16:52 ` Brijesh Singh 2018-02-22 12:08 ` Laszlo Ersek 1 sibling, 1 reply; 11+ messages in thread From: Brijesh Singh @ 2018-02-21 16:52 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel 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 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/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 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 + // 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); + } +} -- 2.14.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-21 16:52 ` [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh @ 2018-02-22 12:08 ` Laszlo Ersek 2018-02-27 12:23 ` Brijesh Singh 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2018-02-22 12:08 UTC (permalink / raw) To: Brijesh Singh, edk2-devel Cc: Tom Lendacky, Jordan Justen, Ard Biesheuvel, Michael Kinney, Paolo Bonzini 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 have more comments: > > 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/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. > 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/ > + // 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. 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".) Thanks, Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-22 12:08 ` Laszlo Ersek @ 2018-02-27 12:23 ` Brijesh Singh 2018-02-27 17:17 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Brijesh Singh @ 2018-02-27 12:23 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Jordan Justen, Ard Biesheuvel, Michael Kinney, Paolo Bonzini 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 <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/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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-27 12:23 ` Brijesh Singh @ 2018-02-27 17:17 ` Laszlo Ersek 2018-02-27 20:37 ` Brijesh Singh 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2018-02-27 17:17 UTC (permalink / raw) To: Brijesh Singh, edk2-devel Cc: Tom Lendacky, Jordan Justen, Ard Biesheuvel, Michael Kinney, Paolo Bonzini Hi Brijesh, you provided a lot of information (and it seems like your analysis was advancing in parallel with your email -- I too do that sometimes :) ), so it's not easy for me to write a concise response. * Regarding the C-bit that covers the relocated save state area (esp. considering that the area is not page aligned and/or contains executable code): I think (a) adding any code (under OvmfPkg, or under the core) that sets the C-bit "correctly", and in turn, (b) lacking any code in edk2 that actually *exercises* the "correct" C-bit setting, is counter-productive. Unless the C-bit is actively exercised, we're just adding *dead* code, which is a bad thing -- it's very easy to regress (without anyone noticing), and it leads to developer confusion. On the other hand, I wouldn't want your analysis to be lost (remember: I asked for the explanation because I didn't understand the behavior). So I'm thinking your analysis should be captured in both a commit message, and a large comment *somewhere*. Possibly near the code (wherever it may end up, AmdSevDxe or SmmCpuFeaturesLib) where you manage the C-bit for the *initial* save state map. I mean, wherever you manage the C-bit for the initial save state map (which is required), you can also make a large comment on the *future* location and behavior. IMO it's OK if we don't guarantee the guest with functional access into the relocated save state map -- there is no actual code relying on that! -- as long as we document this gap. Does this suggestion make sense to you? * Regarding mapping all the NonExistent and MMIO GCD entries in the SMM page table as plaintext: I think we should really be on par with AmdSevDxe here. This is code that *will* be exercised, and if we cut corners here, next time we add another MMIO range or device that needs to be accessed from SMM too (for whatever reason), we'll go crazy otherwise. * In general, regarding how and when SmmCpuFeaturesLib APIs are hooked into PiSmmCpuDxeSmm: please ask questions -- and ask them to Mike :) OVMF's instance of this lib class is Paolo's work (thanks again for that!), so I always defer to Mike and Paolo whenever this lib class and instance come up. SmmCpuFeaturesInitializeProcessor() looked suitable to me, for implementing the previous bullet, but it's really just my "shopping" for a good pre-existent hook point. If we need something better, let's discuss it with Mike. I'm sorry if the above is a bit too vague; please post some new patches, even if only RFCs :) Thanks! Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-27 17:17 ` Laszlo Ersek @ 2018-02-27 20:37 ` Brijesh Singh 2018-02-28 15:21 ` Brijesh Singh 0 siblings, 1 reply; 11+ messages in thread From: Brijesh Singh @ 2018-02-27 20:37 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Jordan Justen, Ard Biesheuvel, Michael Kinney, Paolo Bonzini Hi Laszlo, On 2/27/18 11:17 AM, Laszlo Ersek wrote: > Hi Brijesh, > > you provided a lot of information (and it seems like your analysis was > advancing in parallel with your email -- I too do that sometimes :) ), > so it's not easy for me to write a concise response. > > * Regarding the C-bit that covers the relocated save state area (esp. > considering that the area is not page aligned and/or contains executable > code): > > I think (a) adding any code (under OvmfPkg, or under the core) that sets > the C-bit "correctly", and in turn, (b) lacking any code in edk2 that > actually *exercises* the "correct" C-bit setting, is counter-productive. > Unless the C-bit is actively exercised, we're just adding *dead* code, > which is a bad thing -- it's very easy to regress (without anyone > noticing), and it leads to developer confusion. > > On the other hand, I wouldn't want your analysis to be lost (remember: I > asked for the explanation because I didn't understand the behavior). So > I'm thinking your analysis should be captured in both a commit message, > and a large comment *somewhere*. Possibly near the code (wherever it may > end up, AmdSevDxe or SmmCpuFeaturesLib) where you manage the C-bit for > the *initial* save state map. > > I mean, wherever you manage the C-bit for the initial save state map > (which is required), you can also make a large comment on the *future* > location and behavior. > > IMO it's OK if we don't guarantee the guest with functional access into > the relocated save state map -- there is no actual code relying on that! > -- as long as we document this gap. > > Does this suggestion make sense to you? I am good with this approach. I will add my analysis detail in commit message and also put the similar thing in AmdSevDxe. In future if EDKII makes use to SmmSavedArea after the SMBASE relocation then we can revisit it. > > * Regarding mapping all the NonExistent and MMIO GCD entries in the SMM > page table as plaintext: I think we should really be on par with > AmdSevDxe here. This is code that *will* be exercised, and if we cut > corners here, next time we add another MMIO range or device that needs > to be accessed from SMM too (for whatever reason), we'll go crazy otherwise. Sounds good, I will make the necessary changes and send the update patch. thanks for your help. > > * In general, regarding how and when SmmCpuFeaturesLib APIs are hooked > into PiSmmCpuDxeSmm: please ask questions -- and ask them to Mike :) > OVMF's instance of this lib class is Paolo's work (thanks again for > that!), so I always defer to Mike and Paolo whenever this lib class and > instance come up. SmmCpuFeaturesInitializeProcessor() looked suitable to > me, for implementing the previous bullet, but it's really just my > "shopping" for a good pre-existent hook point. If we need something > better, let's discuss it with Mike. > > I'm sorry if the above is a bit too vague; please post some new patches, > even if only RFCs :) I think your explanation is very clear to me and I am in agreement. Let me work on patches. > > Thanks! > Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 2018-02-27 20:37 ` Brijesh Singh @ 2018-02-28 15:21 ` Brijesh Singh 0 siblings, 0 replies; 11+ messages in thread From: Brijesh Singh @ 2018-02-28 15:21 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Jordan Justen, Ard Biesheuvel, Michael Kinney, Paolo Bonzini Hi Laszlo, On 02/27/2018 02:37 PM, Brijesh Singh wrote: > Hi Laszlo, > > > On 2/27/18 11:17 AM, Laszlo Ersek wrote: >> Hi Brijesh, >> >> you provided a lot of information (and it seems like your analysis was >> advancing in parallel with your email -- I too do that sometimes :) ), >> so it's not easy for me to write a concise response. >> >> * Regarding the C-bit that covers the relocated save state area (esp. >> considering that the area is not page aligned and/or contains executable >> code): >> >> I think (a) adding any code (under OvmfPkg, or under the core) that sets >> the C-bit "correctly", and in turn, (b) lacking any code in edk2 that >> actually *exercises* the "correct" C-bit setting, is counter-productive. >> Unless the C-bit is actively exercised, we're just adding *dead* code, >> which is a bad thing -- it's very easy to regress (without anyone >> noticing), and it leads to developer confusion. >> >> On the other hand, I wouldn't want your analysis to be lost (remember: I >> asked for the explanation because I didn't understand the behavior). So >> I'm thinking your analysis should be captured in both a commit message, >> and a large comment *somewhere*. Possibly near the code (wherever it may >> end up, AmdSevDxe or SmmCpuFeaturesLib) where you manage the C-bit for >> the *initial* save state map. >> >> I mean, wherever you manage the C-bit for the initial save state map >> (which is required), you can also make a large comment on the *future* >> location and behavior. >> >> IMO it's OK if we don't guarantee the guest with functional access into >> the relocated save state map -- there is no actual code relying on that! >> -- as long as we document this gap. >> >> Does this suggestion make sense to you? > > I am good with this approach. I will add my analysis detail in commit > message and also put the similar thing in AmdSevDxe. In future if EDKII > makes use to SmmSavedArea after the SMBASE relocation then we can > revisit it. > >> >> * Regarding mapping all the NonExistent and MMIO GCD entries in the SMM >> page table as plaintext: I think we should really be on par with >> AmdSevDxe here. This is code that *will* be exercised, and if we cut >> corners here, next time we add another MMIO range or device that needs >> to be accessed from SMM too (for whatever reason), we'll go crazy otherwise. > > Sounds good, I will make the necessary changes and send the update > patch. thanks for your help. > OK, I have been trying to implement this and its making things much harder than we thought. It seems SMM page table is very minimal and does not have PTEs for all those MMIO and NonExistent range. So, when we try to clear the C-bit on those range then I have no issue clearing the bits (it just cause more fault) but later in boot we get ASSERTs. I have seen all kind of assertion but the most of time I get below message SMM exception at access (0x7FC01000) It is invoked from the instruction before IP(0x7FFCB6EF) in module (/disk-part-1/upstream/edk2/Build/Ovmf3264/DEBUG_GCC5/X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm/DEBUG/PiSmmCpuDxeSmm.dll) or Sometime SMM page table overlap assertion etc. If we just do Flash range then have no issues. Do you want me to dig more deep on this or you are okay with my previous approach. >> >> * In general, regarding how and when SmmCpuFeaturesLib APIs are hooked >> into PiSmmCpuDxeSmm: please ask questions -- and ask them to Mike :) >> OVMF's instance of this lib class is Paolo's work (thanks again for >> that!), so I always defer to Mike and Paolo whenever this lib class and >> instance come up. SmmCpuFeaturesInitializeProcessor() looked suitable to >> me, for implementing the previous bullet, but it's really just my >> "shopping" for a good pre-existent hook point. If we need something >> better, let's discuss it with Mike. >> >> I'm sorry if the above is a bit too vague; please post some new patches, >> even if only RFCs :) > > I think your explanation is very clear to me and I am in agreement. Let > me work on patches. > >> >> Thanks! >> Laszlo > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-28 15:15 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-21 16:52 [PATCH 0/2] Add SMM support when SEV is active Brijesh Singh 2018-02-21 16:52 ` [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State Brijesh Singh 2018-02-22 11:20 ` Laszlo Ersek 2018-02-22 11:21 ` Laszlo Ersek 2018-02-27 12:17 ` Brijesh Singh 2018-02-21 16:52 ` [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active Brijesh Singh 2018-02-22 12:08 ` Laszlo Ersek 2018-02-27 12:23 ` Brijesh Singh 2018-02-27 17:17 ` Laszlo Ersek 2018-02-27 20:37 ` Brijesh Singh 2018-02-28 15:21 ` Brijesh Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox