Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
My understading: Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf is DXE counterpart of Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmm.inf, to produce EfiFirmwareVolumeBlock2?ProtocolGuid based on gEfiSmmFirmwareVolumeBlockProtocolGuid by SMM comm. Platform can choose FvbSmmDxe + FvbSmm, or FvbRuntimeDxe + FvbSmm. Thanks, Star -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, June 28, 2018 9:14 PM To: Zeng, Star ; Brijesh Singh ; edk2-devel@lists.01.org Cc: Tom Lendacky ; Dong, Eric ; Justen, Jordan L Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled On 06/28/18 08:16, Zeng, Star wrote: > 1) My understanding is Variable Driver is managing the variable region in > flash although the flash read/write/erase operations are done in flash > driver. Current Variable Driver needs the address (to variable region) be > converted to virtual address for runtime, and it does not assume flash driver > to set the runtime attribute for variable region, but do it by itself. > > 2) I agree this approach. If the runtime attribute has been set by flash > driver, Variable Driver has no need to do that. Great, thank you! Yesterday, after I sent my email(s), I got concerned for a moment, because it wasn't clear to me what *runtime driver* actually owned the flash range. However, the FVB protocol itself must be provided by a runtime driver on UEFI/PI platforms where the variable write service implementation -- indirectly or directly -- depends on FVB, for flash writes. Indeed in edk2, we have at least the following FVB drivers: $ git grep -l -E \ -e 'EfiFirmwareVolumeBlock2?ProtocolGuid' --and -e 'PROD' EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf Nt32Pkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbRuntimeDxe.inf Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf All of them are DXE_RUNTIME_DRIVER modules [*] and they all call EfiConvertPointer(). [*] "Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf" is marked as a DXE_DRIVER (not runtime), and indeed it doesn't call EfiConvertPointer(). I don't know what this driver is good for; the INF file is not included in any DSC file in edk2. I guess it can be used for flash access on an SMM platform as long as you never boot an OS / never call ExitBootServices(). In that case however, runtime aspects have no meaning at all. Thanks! Laszlo > > > Thanks, > Star > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Wednesday, June 27, 2018 8:54 PM > To: Brijesh Singh ; edk2-devel@lists.01.org > Cc: Tom Lendacky ; Dong, Eric > ; Zeng, Star ; Justen, > Jordan L > Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime > variable access when SEV is enabled > > On 06/26/18 21:46, Brijesh Singh wrote: >> Problem statement: >> -- >> Fedora-28 contains 4.16 kernel -- which has all the required support >> to run as an SEV guest. When the installer is launched from SEV >> guest then it fails to install the bootloader. The installer was >> failing to update the 'BootOrder' UEFI runtime variable. >> >> Root Cause Analysis >> >> Since QemuFlash storage memory is accessed by both guest and >> hypervisor hence we need to map this memory range as unencrypted. >> AmdSevDxe maps the range as "unencrypted" but later >> FtwNotificationEvent() in >> MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping and >> the memory region gets remapped as "encrypted". > > Is this a new issue, or has it always been there, and we just failed to > notice it? > > BTW, I don't understand why FtwNotificationEvent() in > "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c" has to mark the > flash range as EFI_MEMORY_RUNTIME. I thought that this action belonged in the > flash driver itself, and we do that already in > MarkMemoryRangeForRuntimeAccess(), in file > "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". > > The variable driver does not own the pflash chip (the pflash driver owns it), > so I believe the variable driver shouldn't mess with the mapping attributes. > > Here's a suggestion -- Star, Eric, can you please comment? In the > FtwNotificationEvent() function, after we get the memory descriptor for the > pflash range, first check whether EFI_MEMORY_RUNTIME is already set. > If it is, don't do anything; if it isn't, add the attribute. > > This should cause no ob
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/28/2018 07:57 AM, Laszlo Ersek wrote: [...] --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess ( ); Status = gDS->AddMemorySpace ( - EfiGcdMemoryTypeSystemMemory, + EfiGcdMemoryTypeMemoryMappedIo, BaseAddress, Length, EFI_MEMORY_UC | EFI_MEMORY_RUNTIME @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess ( Status = gBS->AllocatePages ( AllocateAddress, - EfiRuntimeServicesData, + EfiMemoryMappedIO, EFI_SIZE_TO_PAGES (Length), ); I am still getting the error assertion failure. I can debug to see what is going on. Hmmm. Indeed, memory space added to GCD need not immediately show up in the UEFI memory map, for the UEFI memory allocation services to allocate from. IIRC, the PI spec says that *system memory* added like this *may* show up immediately: """ If the memory range specified by BaseAddress and Length is of type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the memory range may be automatically allocated for use by the UEFI memory services. """ and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at all. So, can you replace the AllocatePages call with the following: Status = gDS->AllocateMemorySpace ( EfiGcdAllocateAddress, EfiGcdMemoryTypeMemoryMappedIo, 0, // Alignment EFI_SIZE_TO_PAGES (Length), , gImageHandle, NULL// DeviceHandle ); I did realized this in my yesterday's debug. I was looking at other drivers (mainly PciBridge...) on how it adds the MMIO. I no longer get the ASSERT. thank you ! After changing this system boots fine but I am getting Linux kernel crashes when we attempt to update the UEFI runtime variables. I am still debugging and trying to to root cause it. Good point, I will try it and let you know. As you say since SMM uses UEFI page table More correctly: SMM drivers use, at runtime as well, the page table in SMRAM that was created by the firmware. hence after fixing FtwNotificationEvent(..) we should be good. No, that's not precise -- FtwNotificationEvent() is not used in SMM *at all*. I checked that before I sent my previous email; namely, FtwNotificationEvent() is in "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c", which is not built into the SMM variant of the variable driver. Please see the variable driver stacks in OVMF: - non-SMM: OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf In the non-SMM case, we have a normal runtime driver, VariableRuntimeDxe.inf, that implements the variable service. It relies on the FTW driver (another runtime driver) for the FTW protocol. Finally, both the variable driver and the FTW driver rely on the FVB (firmware volume block) protocol, which may come from the QEMU flash driver, or from the emulation driver. Again, all of these are runtime drivers. "VariableRuntimeDxe.inf" includes the "VariableDxe.c" file, hence FtwNotificationEvent() is relevant. - SMM: OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf In the SMM case, we have a *split* variable driver, "VariableSmmRuntimeDxe.inf" (which is a normal runtime driver) and "VariableSmm.inf" (a "traditional SMM" driver). The runtime half is unprivileged and only formats a message for the privileged SMM half, submits the message, and then parses the response. The rest of the variable stack, namely the privileged half of the variable driver, and the FTW driver, and the QEMU flash (FVB) driver, all stay within SMM -- they are traditional SMM drivers too. "VariableSmmRuntimeDxe.inf" does not include "VariableDxe.c". Instead, "VariableSmm.inf" (the privileged half) includes "VariableSmm.c", and that source file waits for the *SMM* FVB protocol, in the SmmFtwNotificationEvent() function. But, this notification function, unlike the DXE counterpart FtwNotificationEvent(), does not care about the GCD entry attributes -- because in SMM, separate (protected) page tables are used anyway. If you put FtwNotificationEvent() and SmmFtwNotificationEvent() side by side, you'll see that they perform almost identical steps: - "Ensure [SMM] FTW
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/28/18 14:57, Laszlo Ersek wrote: > On 06/27/18 19:49, Brijesh Singh wrote: >> >> >> On 06/27/2018 11:59 AM, Laszlo Ersek wrote: >>> On 06/27/18 18:34, Brijesh Singh wrote: On 06/27/2018 07:54 AM, Laszlo Ersek wrote: > On 06/26/18 21:46, Brijesh Singh wrote: >>> >> After that, any access >> to the flash will end up going through the encryption engine. I did >> try >> hacking EDK2 to restore the C-bit > > (I continue to be annoyed that the memory encryption bit is not exposed > in the GCD memory space attributes explicitly.) > >> but that was not sufficient because UEFI >> runtime services are mapped as "encrypted" in OS page table > > What do you mean here? Runtime services *code* or runtime services > *data*? Code must obviously be remain encrypted (otherwise we cannot > execute it in SEV). Runtime Services Data should also be mapped as > encrypted (it is normal RAM that is not used for guest<->hypervisor > exchange). Sorry, I was meaning to say both the "code" and "data" are mapped as encrypted by the OS. >> hence we end up accessing the flash as encrypted when OS requests to >> update the variables. > > I don't understand the "hence" here; I don't see how the implication > follows. runtime services code and data should be encrypted. Runtime > MMIO should be un-encrypted. > > Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use > "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good > idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. Right, the memory is marked as 'system ram' and not 'mmio'. Just to experiment, I did try changing it to 'mmio' to see if OS will map this region as "unencrypted" but ovmf fails with below error message after changing it from systemRAM->mmio ConvertPages: failed to find range FFC0 - ASSERT_EFI_ERROR (Status = Not Found) ASSERT [FvbServicesRuntimeDxe] /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): !EFI_ERROR (Status) >>> >>> This error occurs because (I think) you modified only the AddMemorySpace >>> call. If you change the GCD type on that, then please update the >>> subsequent AllocatePages as well, from EfiRuntimeServicesData to >>> EfiMemoryMappedIO. >>> >> >> Here is what I have. >> >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess ( >> ); >> >> Status = gDS->AddMemorySpace ( >> - EfiGcdMemoryTypeSystemMemory, >> + EfiGcdMemoryTypeMemoryMappedIo, >> BaseAddress, >> Length, >> EFI_MEMORY_UC | EFI_MEMORY_RUNTIME >> @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess ( >> >> Status = gBS->AllocatePages ( >> AllocateAddress, >> - EfiRuntimeServicesData, >> + EfiMemoryMappedIO, >> EFI_SIZE_TO_PAGES (Length), >> >> ); >> >> I am still getting the error assertion failure. I can debug to see what >> is going on. > > Hmmm. Indeed, memory space added to GCD need not immediately show up in > the UEFI memory map, for the UEFI memory allocation services to allocate > from. IIRC, the PI spec says that *system memory* added like this *may* > show up immediately: > > """ > If the memory range specified by BaseAddress and Length is of type > EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the > memory range may be automatically allocated for use by the UEFI memory > services. > """ > > and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at > all. > > So, can you replace the AllocatePages call with the following: > > Status = gDS->AllocateMemorySpace ( > EfiGcdAllocateAddress, > EfiGcdMemoryTypeMemoryMappedIo, > 0, // Alignment > EFI_SIZE_TO_PAGES (Length), > , > gImageHandle, > NULL// DeviceHandle > ); > >>> The spec says about the latter enum constant, "Used by system firmware >>> to request that a memory-mapped IO region be mapped by the OS to a >>> virtual address so it can be accessed by EFI runtime services." It seems >>> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and >>> UEFI enum values for the memory type, all this time.) >>> Since this efi runtime data is mapped as C=1 by the OS, hence when OS asks efi to update the runtime variable we end up accessing the memory region with C=1 (runtime services are executed using OS pagetable). >>> >>> Indeed. >>> >>> (And,
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/28/18 08:25, Zeng, Star wrote: > My understanding is MMIO is not managed by UEFI memory services, but GCD > services. > PI spec says " If the memory range specified by BaseAddress and Length is of > type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the > memory range may be automatically *allocated* for use by the *UEFI memory > services*." in AddMemorySpace() description. > > For MMIO, the code needs to use AddMemorySpace() + AllocateMemorySpace(). Right, after seeing today the (updated) AllocatePages() failure, reported by Brijesh, I came to the same conclusion. Thank you for correcting my initial suggestion! It's not easy to keep this details in mind :) Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/28/18 08:16, Zeng, Star wrote: > 1) My understanding is Variable Driver is managing the variable region in > flash although the flash read/write/erase operations are done in flash > driver. Current Variable Driver needs the address (to variable region) be > converted to virtual address for runtime, and it does not assume flash driver > to set the runtime attribute for variable region, but do it by itself. > > 2) I agree this approach. If the runtime attribute has been set by flash > driver, Variable Driver has no need to do that. Great, thank you! Yesterday, after I sent my email(s), I got concerned for a moment, because it wasn't clear to me what *runtime driver* actually owned the flash range. However, the FVB protocol itself must be provided by a runtime driver on UEFI/PI platforms where the variable write service implementation -- indirectly or directly -- depends on FVB, for flash writes. Indeed in edk2, we have at least the following FVB drivers: $ git grep -l -E \ -e 'EfiFirmwareVolumeBlock2?ProtocolGuid' --and -e 'PROD' EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf Nt32Pkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbRuntimeDxe.inf Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf All of them are DXE_RUNTIME_DRIVER modules [*] and they all call EfiConvertPointer(). [*] "Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf" is marked as a DXE_DRIVER (not runtime), and indeed it doesn't call EfiConvertPointer(). I don't know what this driver is good for; the INF file is not included in any DSC file in edk2. I guess it can be used for flash access on an SMM platform as long as you never boot an OS / never call ExitBootServices(). In that case however, runtime aspects have no meaning at all. Thanks! Laszlo > > > Thanks, > Star > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Wednesday, June 27, 2018 8:54 PM > To: Brijesh Singh ; edk2-devel@lists.01.org > Cc: Tom Lendacky ; Dong, Eric ; > Zeng, Star ; Justen, Jordan L > Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable > access when SEV is enabled > > On 06/26/18 21:46, Brijesh Singh wrote: >> Problem statement: >> -- >> Fedora-28 contains 4.16 kernel -- which has all the required support >> to run as an SEV guest. When the installer is launched from SEV guest >> then it fails to install the bootloader. The installer was failing to >> update the 'BootOrder' UEFI runtime variable. >> >> Root Cause Analysis >> >> Since QemuFlash storage memory is accessed by both guest and >> hypervisor hence we need to map this memory range as unencrypted. >> AmdSevDxe maps the range as "unencrypted" but later >> FtwNotificationEvent() in >> MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping and >> the memory region gets remapped as "encrypted". > > Is this a new issue, or has it always been there, and we just failed to > notice it? > > BTW, I don't understand why FtwNotificationEvent() in > "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c" has to mark the > flash range as EFI_MEMORY_RUNTIME. I thought that this action belonged in the > flash driver itself, and we do that already in > MarkMemoryRangeForRuntimeAccess(), in file > "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". > > The variable driver does not own the pflash chip (the pflash driver owns it), > so I believe the variable driver shouldn't mess with the mapping attributes. > > Here's a suggestion -- Star, Eric, can you please comment? In the > FtwNotificationEvent() function, after we get the memory descriptor for the > pflash range, first check whether EFI_MEMORY_RUNTIME is already set. > If it is, don't do anything; if it isn't, add the attribute. > > This should cause no observable change on any non-SEV platform, and it should > remove the gDS->SetMemorySpaceAttributes() call for OVMF (where it breaks > things for SEV). > >> After that, any access >> to the flash will end up going through the encryption engine. I did >> try hacking EDK2 to restore the C-bit > > (I continue to be annoyed that the memory encryption bit is not exposed in > the GCD memory space attributes explicitly.) > >> but that was not sufficient because UEFI runtime services are mapped >> as "encrypted" in OS page table > > What do you mean here? Runtime services *code* or runtime services *data*? > Code must obvio
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/27/18 19:49, Brijesh Singh wrote: > > > On 06/27/2018 11:59 AM, Laszlo Ersek wrote: >> On 06/27/18 18:34, Brijesh Singh wrote: >>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote: On 06/26/18 21:46, Brijesh Singh wrote: >> > After that, any access > to the flash will end up going through the encryption engine. I did > try > hacking EDK2 to restore the C-bit (I continue to be annoyed that the memory encryption bit is not exposed in the GCD memory space attributes explicitly.) > but that was not sufficient because UEFI > runtime services are mapped as "encrypted" in OS page table What do you mean here? Runtime services *code* or runtime services *data*? Code must obviously be remain encrypted (otherwise we cannot execute it in SEV). Runtime Services Data should also be mapped as encrypted (it is normal RAM that is not used for guest<->hypervisor exchange). >>> >>> Sorry, I was meaning to say both the "code" and "data" are mapped as >>> encrypted by the OS. >>> > hence we end up accessing the flash as encrypted when OS requests to > update the variables. I don't understand the "hence" here; I don't see how the implication follows. runtime services code and data should be encrypted. Runtime MMIO should be un-encrypted. Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. >>> >>> Right, the memory is marked as 'system ram' and not 'mmio'. >>> Just to experiment, I did try changing it to 'mmio' to see if OS will >>> map this region as "unencrypted" but ovmf fails with below error >>> message after changing it from systemRAM->mmio >>> >>> ConvertPages: failed to find range FFC0 - >>> ASSERT_EFI_ERROR (Status = Not Found) >>> ASSERT [FvbServicesRuntimeDxe] >>> /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): >>> >>> !EFI_ERROR (Status) >> >> This error occurs because (I think) you modified only the AddMemorySpace >> call. If you change the GCD type on that, then please update the >> subsequent AllocatePages as well, from EfiRuntimeServicesData to >> EfiMemoryMappedIO. >> > > Here is what I have. > > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess ( > ); > > Status = gDS->AddMemorySpace ( > - EfiGcdMemoryTypeSystemMemory, > + EfiGcdMemoryTypeMemoryMappedIo, > BaseAddress, > Length, > EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess ( > > Status = gBS->AllocatePages ( > AllocateAddress, > - EfiRuntimeServicesData, > + EfiMemoryMappedIO, > EFI_SIZE_TO_PAGES (Length), > > ); > > I am still getting the error assertion failure. I can debug to see what > is going on. Hmmm. Indeed, memory space added to GCD need not immediately show up in the UEFI memory map, for the UEFI memory allocation services to allocate from. IIRC, the PI spec says that *system memory* added like this *may* show up immediately: """ If the memory range specified by BaseAddress and Length is of type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the memory range may be automatically allocated for use by the UEFI memory services. """ and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at all. So, can you replace the AllocatePages call with the following: Status = gDS->AllocateMemorySpace ( EfiGcdAllocateAddress, EfiGcdMemoryTypeMemoryMappedIo, 0, // Alignment EFI_SIZE_TO_PAGES (Length), , gImageHandle, NULL// DeviceHandle ); >> The spec says about the latter enum constant, "Used by system firmware >> to request that a memory-mapped IO region be mapped by the OS to a >> virtual address so it can be accessed by EFI runtime services." It seems >> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and >> UEFI enum values for the memory type, all this time.) >> >>> Since this efi runtime data is mapped as C=1 by the OS, hence when OS >>> asks efi to update the runtime variable we end up accessing the memory >>> region with C=1 (runtime services are executed using OS pagetable). >> >> Indeed. >> >> (And, this is only a problem when SMM is not used, i.e. when the full >> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM >> page tables are used, and the OS cannot
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
My understanding is MMIO is not managed by UEFI memory services, but GCD services. PI spec says " If the memory range specified by BaseAddress and Length is of type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the memory range may be automatically *allocated* for use by the *UEFI memory services*." in AddMemorySpace() description. For MMIO, the code needs to use AddMemorySpace() + AllocateMemorySpace(). Thanks, Star -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Brijesh Singh Sent: Thursday, June 28, 2018 1:50 AM To: Laszlo Ersek ; edk2-devel@lists.01.org Cc: Tom Lendacky ; brijesh.si...@amd.com; Dong, Eric ; Zeng, Star ; Justen, Jordan L Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled On 06/27/2018 11:59 AM, Laszlo Ersek wrote: > On 06/27/18 18:34, Brijesh Singh wrote: >> On 06/27/2018 07:54 AM, Laszlo Ersek wrote: >>> On 06/26/18 21:46, Brijesh Singh wrote: > >>>> After that, any access >>>> to the flash will end up going through the encryption engine. I did >>>> try hacking EDK2 to restore the C-bit >>> >>> (I continue to be annoyed that the memory encryption bit is not >>> exposed in the GCD memory space attributes explicitly.) >>> >>>> but that was not sufficient because UEFI runtime services are >>>> mapped as "encrypted" in OS page table >>> >>> What do you mean here? Runtime services *code* or runtime services >>> *data*? Code must obviously be remain encrypted (otherwise we cannot >>> execute it in SEV). Runtime Services Data should also be mapped as >>> encrypted (it is normal RAM that is not used for guest<->hypervisor >>> exchange). >> >> Sorry, I was meaning to say both the "code" and "data" are mapped as >> encrypted by the OS. >> >>>> hence we end up accessing the flash as encrypted when OS requests >>>> to update the variables. >>> >>> I don't understand the "hence" here; I don't see how the implication >>> follows. runtime services code and data should be encrypted. Runtime >>> MMIO should be un-encrypted. >>> >>> Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use >>> "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a >>> good idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. >> >> Right, the memory is marked as 'system ram' and not 'mmio'. >> Just to experiment, I did try changing it to 'mmio' to see if OS will >> map this region as "unencrypted" but ovmf fails with below error >> message after changing it from systemRAM->mmio >> >> ConvertPages: failed to find range FFC0 - >> ASSERT_EFI_ERROR (Status = Not Found) ASSERT [FvbServicesRuntimeDxe] >> /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): >> !EFI_ERROR (Status) > > This error occurs because (I think) you modified only the > AddMemorySpace call. If you change the GCD type on that, then please > update the subsequent AllocatePages as well, from > EfiRuntimeServicesData to EfiMemoryMappedIO. > Here is what I have. --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess ( ); Status = gDS->AddMemorySpace ( - EfiGcdMemoryTypeSystemMemory, + EfiGcdMemoryTypeMemoryMappedIo, BaseAddress, Length, EFI_MEMORY_UC | EFI_MEMORY_RUNTIME @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess ( Status = gBS->AllocatePages ( AllocateAddress, - EfiRuntimeServicesData, + EfiMemoryMappedIO, EFI_SIZE_TO_PAGES (Length), ); I am still getting the error assertion failure. I can debug to see what is going on. > The spec says about the latter enum constant, "Used by system firmware > to request that a memory-mapped IO region be mapped by the OS to a > virtual address so it can be accessed by EFI runtime services." It seems > appropriate (and I'm a bit confused why we haven't used the MMIO GCD and > UEFI enum values for the memory type, all this time.) > >> Since this efi runtime data is mapped as C=1 by the OS, hence when OS >> asks efi to update the runtime variable we end up accessing the memory >> region with C=1 (runtime services are executed using OS pageta
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
1) My understanding is Variable Driver is managing the variable region in flash although the flash read/write/erase operations are done in flash driver. Current Variable Driver needs the address (to variable region) be converted to virtual address for runtime, and it does not assume flash driver to set the runtime attribute for variable region, but do it by itself. 2) I agree this approach. If the runtime attribute has been set by flash driver, Variable Driver has no need to do that. Thanks, Star -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Wednesday, June 27, 2018 8:54 PM To: Brijesh Singh ; edk2-devel@lists.01.org Cc: Tom Lendacky ; Dong, Eric ; Zeng, Star ; Justen, Jordan L Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled On 06/26/18 21:46, Brijesh Singh wrote: > Problem statement: > -- > Fedora-28 contains 4.16 kernel -- which has all the required support > to run as an SEV guest. When the installer is launched from SEV guest > then it fails to install the bootloader. The installer was failing to > update the 'BootOrder' UEFI runtime variable. > > Root Cause Analysis > > Since QemuFlash storage memory is accessed by both guest and > hypervisor hence we need to map this memory range as unencrypted. > AmdSevDxe maps the range as "unencrypted" but later > FtwNotificationEvent() in > MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping and > the memory region gets remapped as "encrypted". Is this a new issue, or has it always been there, and we just failed to notice it? BTW, I don't understand why FtwNotificationEvent() in "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c" has to mark the flash range as EFI_MEMORY_RUNTIME. I thought that this action belonged in the flash driver itself, and we do that already in MarkMemoryRangeForRuntimeAccess(), in file "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". The variable driver does not own the pflash chip (the pflash driver owns it), so I believe the variable driver shouldn't mess with the mapping attributes. Here's a suggestion -- Star, Eric, can you please comment? In the FtwNotificationEvent() function, after we get the memory descriptor for the pflash range, first check whether EFI_MEMORY_RUNTIME is already set. If it is, don't do anything; if it isn't, add the attribute. This should cause no observable change on any non-SEV platform, and it should remove the gDS->SetMemorySpaceAttributes() call for OVMF (where it breaks things for SEV). > After that, any access > to the flash will end up going through the encryption engine. I did > try hacking EDK2 to restore the C-bit (I continue to be annoyed that the memory encryption bit is not exposed in the GCD memory space attributes explicitly.) > but that was not sufficient because UEFI runtime services are mapped > as "encrypted" in OS page table What do you mean here? Runtime services *code* or runtime services *data*? Code must obviously be remain encrypted (otherwise we cannot execute it in SEV). Runtime Services Data should also be mapped as encrypted (it is normal RAM that is not used for guest<->hypervisor exchange). > hence we end up accessing the flash as encrypted when OS requests to update > the variables. I don't understand the "hence" here; I don't see how the implication follows. runtime services code and data should be encrypted. Runtime MMIO should be un-encrypted. Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. ... Anyway, I think first we should go with the "check EFI_MEMORY_RUNTIME attribute before setting it", in FtwNotificationEvent(). > > A possible solution > - > To solve the issue, after QemuFlash is probed, I allocate an encrypted > buffer and initialize this buffer with the contents from the flash memory. > When SEV is enabled, we use newly allocated encrypted buffer in > FwInstance->FvBase instead of the original flash region. The idea is > FwInstance->if > caller grabs the FwInstance->FvBase pointer and tries to access the > FvVolumeHeader then it should get the data from the encrypted buffer. > But if caller wants read/writes to/from the flash device then we > internally use the original "unencrypted" flash region to access the data. No, this is neither safe, nor a desirable design. Safety: all accesses (via both pointers and FVB protocol members) that higher-level drives *think* go to the pflash chip must *actually* go to the pflash chip. Design: it
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/27/2018 11:59 AM, Laszlo Ersek wrote: On 06/27/18 18:34, Brijesh Singh wrote: On 06/27/2018 07:54 AM, Laszlo Ersek wrote: On 06/26/18 21:46, Brijesh Singh wrote: After that, any access to the flash will end up going through the encryption engine. I did try hacking EDK2 to restore the C-bit (I continue to be annoyed that the memory encryption bit is not exposed in the GCD memory space attributes explicitly.) but that was not sufficient because UEFI runtime services are mapped as "encrypted" in OS page table What do you mean here? Runtime services *code* or runtime services *data*? Code must obviously be remain encrypted (otherwise we cannot execute it in SEV). Runtime Services Data should also be mapped as encrypted (it is normal RAM that is not used for guest<->hypervisor exchange). Sorry, I was meaning to say both the "code" and "data" are mapped as encrypted by the OS. hence we end up accessing the flash as encrypted when OS requests to update the variables. I don't understand the "hence" here; I don't see how the implication follows. runtime services code and data should be encrypted. Runtime MMIO should be un-encrypted. Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. Right, the memory is marked as 'system ram' and not 'mmio'. Just to experiment, I did try changing it to 'mmio' to see if OS will map this region as "unencrypted" but ovmf fails with below error message after changing it from systemRAM->mmio ConvertPages: failed to find range FFC0 - ASSERT_EFI_ERROR (Status = Not Found) ASSERT [FvbServicesRuntimeDxe] /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): !EFI_ERROR (Status) This error occurs because (I think) you modified only the AddMemorySpace call. If you change the GCD type on that, then please update the subsequent AllocatePages as well, from EfiRuntimeServicesData to EfiMemoryMappedIO. Here is what I have. --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess ( ); Status = gDS->AddMemorySpace ( - EfiGcdMemoryTypeSystemMemory, + EfiGcdMemoryTypeMemoryMappedIo, BaseAddress, Length, EFI_MEMORY_UC | EFI_MEMORY_RUNTIME @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess ( Status = gBS->AllocatePages ( AllocateAddress, - EfiRuntimeServicesData, + EfiMemoryMappedIO, EFI_SIZE_TO_PAGES (Length), ); I am still getting the error assertion failure. I can debug to see what is going on. The spec says about the latter enum constant, "Used by system firmware to request that a memory-mapped IO region be mapped by the OS to a virtual address so it can be accessed by EFI runtime services." It seems appropriate (and I'm a bit confused why we haven't used the MMIO GCD and UEFI enum values for the memory type, all this time.) Since this efi runtime data is mapped as C=1 by the OS, hence when OS asks efi to update the runtime variable we end up accessing the memory region with C=1 (runtime services are executed using OS pagetable). Indeed. (And, this is only a problem when SMM is not used, i.e. when the full variable driver stack is non-SMM, just DXE. In the SMM case, the SMM page tables are used, and the OS cannot interfere with that.) Good point, I will try it and let you know. As you say since SMM uses UEFI page table hence after fixing FtwNotificationEvent(..) we should be good. Anyway, in the pure DXE / runtime driver case, do you think a guest kernel patch will be necessary too? Perhaps if you change the UEFI memmap entry type (see AllocatePages above) to MMIO, then the guest kernel could technically honor that. Theoretically speaking, if we are able to make this memory region as mmio then OS should be able to map it with C=0. -Brijesh ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/27/18 18:34, Brijesh Singh wrote: > On 06/27/2018 07:54 AM, Laszlo Ersek wrote: >> On 06/26/18 21:46, Brijesh Singh wrote: >>> After that, any access >>> to the flash will end up going through the encryption engine. I did try >>> hacking EDK2 to restore the C-bit >> >> (I continue to be annoyed that the memory encryption bit is not exposed >> in the GCD memory space attributes explicitly.) >> >>> but that was not sufficient because UEFI >>> runtime services are mapped as "encrypted" in OS page table >> >> What do you mean here? Runtime services *code* or runtime services >> *data*? Code must obviously be remain encrypted (otherwise we cannot >> execute it in SEV). Runtime Services Data should also be mapped as >> encrypted (it is normal RAM that is not used for guest<->hypervisor >> exchange). > > Sorry, I was meaning to say both the "code" and "data" are mapped as > encrypted by the OS. > >>> hence we end up accessing the flash as encrypted when OS requests to >>> update the variables. >> >> I don't understand the "hence" here; I don't see how the implication >> follows. runtime services code and data should be encrypted. Runtime >> MMIO should be un-encrypted. >> >> Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use >> "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good >> idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. > > Right, the memory is marked as 'system ram' and not 'mmio'. > Just to experiment, I did try changing it to 'mmio' to see if OS will > map this region as "unencrypted" but ovmf fails with below error > message after changing it from systemRAM->mmio > > ConvertPages: failed to find range FFC0 - > ASSERT_EFI_ERROR (Status = Not Found) > ASSERT [FvbServicesRuntimeDxe] > /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): > !EFI_ERROR (Status) This error occurs because (I think) you modified only the AddMemorySpace call. If you change the GCD type on that, then please update the subsequent AllocatePages as well, from EfiRuntimeServicesData to EfiMemoryMappedIO. The spec says about the latter enum constant, "Used by system firmware to request that a memory-mapped IO region be mapped by the OS to a virtual address so it can be accessed by EFI runtime services." It seems appropriate (and I'm a bit confused why we haven't used the MMIO GCD and UEFI enum values for the memory type, all this time.) > Since this efi runtime data is mapped as C=1 by the OS, hence when OS > asks efi to update the runtime variable we end up accessing the memory > region with C=1 (runtime services are executed using OS pagetable). Indeed. (And, this is only a problem when SMM is not used, i.e. when the full variable driver stack is non-SMM, just DXE. In the SMM case, the SMM page tables are used, and the OS cannot interfere with that.) Anyway, in the pure DXE / runtime driver case, do you think a guest kernel patch will be necessary too? Perhaps if you change the UEFI memmap entry type (see AllocatePages above) to MMIO, then the guest kernel could technically honor that. Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/27/2018 11:34 AM, Brijesh Singh wrote: I think (2) will solve the complete issue, we still need to figure how I meant to say (2) will *not* solve the complete issue ! ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
Thanks for the quick feedback Laszlo ! On 06/27/2018 07:54 AM, Laszlo Ersek wrote: On 06/26/18 21:46, Brijesh Singh wrote: Problem statement: -- Fedora-28 contains 4.16 kernel -- which has all the required support to run as an SEV guest. When the installer is launched from SEV guest then it fails to install the bootloader. The installer was failing to update the 'BootOrder' UEFI runtime variable. Root Cause Analysis Since QemuFlash storage memory is accessed by both guest and hypervisor hence we need to map this memory range as unencrypted. AmdSevDxe maps the range as "unencrypted" but later FtwNotificationEvent() in MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping and the memory region gets remapped as "encrypted". Is this a new issue, or has it always been there, and we just failed to notice it? The issue has been always there and we never noticed. I noticed it after I saw installation failure. I can easily reproduce it with simple efibootmgr command: 'efibootmgr -o 0003,0004' -- e.g change boot order BTW, I don't understand why FtwNotificationEvent() in "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c" has to mark the flash range as EFI_MEMORY_RUNTIME. I thought that this action belonged in the flash driver itself, and we do that already in MarkMemoryRangeForRuntimeAccess(), in file "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". The variable driver does not own the pflash chip (the pflash driver owns it), so I believe the variable driver shouldn't mess with the mapping attributes. Here's a suggestion -- Star, Eric, can you please comment? In the FtwNotificationEvent() function, after we get the memory descriptor for the pflash range, first check whether EFI_MEMORY_RUNTIME is already set. If it is, don't do anything; if it isn't, add the attribute. This should cause no observable change on any non-SEV platform, and it should remove the gDS->SetMemorySpaceAttributes() call for OVMF (where it breaks things for SEV). If Star and Eric agrees with your proposal then I can create a patch to fix this issue. After that, any access to the flash will end up going through the encryption engine. I did try hacking EDK2 to restore the C-bit (I continue to be annoyed that the memory encryption bit is not exposed in the GCD memory space attributes explicitly.) but that was not sufficient because UEFI runtime services are mapped as "encrypted" in OS page table What do you mean here? Runtime services *code* or runtime services *data*? Code must obviously be remain encrypted (otherwise we cannot execute it in SEV). Runtime Services Data should also be mapped as encrypted (it is normal RAM that is not used for guest<->hypervisor exchange). Sorry, I was meaning to say both the "code" and "data" are mapped as encrypted by the OS. hence we end up accessing the flash as encrypted when OS requests to update the variables. I don't understand the "hence" here; I don't see how the implication follows. runtime services code and data should be encrypted. Runtime MMIO should be un-encrypted. Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. Right, the memory is marked as 'system ram' and not 'mmio'. Just to experiment, I did try changing it to 'mmio' to see if OS will map this region as "unencrypted" but ovmf fails with below error message after changing it from systemRAM->mmio ConvertPages: failed to find range FFC0 - ASSERT_EFI_ERROR (Status = Not Found) ASSERT [FvbServicesRuntimeDxe] /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): !EFI_ERROR (Status) Since this efi runtime data is mapped as C=1 by the OS, hence when OS asks efi to update the runtime variable we end up accessing the memory region with C=1 (runtime services are executed using OS pagetable). ... Anyway, I think first we should go with the "check EFI_MEMORY_RUNTIME attribute before setting it", in FtwNotificationEvent(). A possible solution - To solve the issue, after QemuFlash is probed, I allocate an encrypted buffer and initialize this buffer with the contents from the flash memory. When SEV is enabled, we use newly allocated encrypted buffer in FwInstance->FvBase instead of the original flash region. The idea is if caller grabs the FwInstance->FvBase pointer and tries to access the FvVolumeHeader then it should get the data from the encrypted buffer. But if caller wants read/writes to/from the flash device then we internally use the original "unencrypted" flash region to access the data. No, this is neither safe, nor a desirable design. While hacking this I knew it was not an idle approach but wanted to start the discussion to find an acceptable solution. Safety: all accesses (via both pointers and FVB
Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
On 06/26/18 21:46, Brijesh Singh wrote: > Problem statement: > -- > Fedora-28 contains 4.16 kernel -- which has all the required support to > run as an SEV guest. When the installer is launched from SEV guest then > it fails to install the bootloader. The installer was failing to update > the 'BootOrder' UEFI runtime variable. > > Root Cause Analysis > > Since QemuFlash storage memory is accessed by both guest and hypervisor > hence we need to map this memory range as unencrypted. AmdSevDxe maps the > range as "unencrypted" but later FtwNotificationEvent() in > MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping > and the memory region gets remapped as "encrypted". Is this a new issue, or has it always been there, and we just failed to notice it? BTW, I don't understand why FtwNotificationEvent() in "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c" has to mark the flash range as EFI_MEMORY_RUNTIME. I thought that this action belonged in the flash driver itself, and we do that already in MarkMemoryRangeForRuntimeAccess(), in file "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". The variable driver does not own the pflash chip (the pflash driver owns it), so I believe the variable driver shouldn't mess with the mapping attributes. Here's a suggestion -- Star, Eric, can you please comment? In the FtwNotificationEvent() function, after we get the memory descriptor for the pflash range, first check whether EFI_MEMORY_RUNTIME is already set. If it is, don't do anything; if it isn't, add the attribute. This should cause no observable change on any non-SEV platform, and it should remove the gDS->SetMemorySpaceAttributes() call for OVMF (where it breaks things for SEV). > After that, any access > to the flash will end up going through the encryption engine. I did try > hacking EDK2 to restore the C-bit (I continue to be annoyed that the memory encryption bit is not exposed in the GCD memory space attributes explicitly.) > but that was not sufficient because UEFI > runtime services are mapped as "encrypted" in OS page table What do you mean here? Runtime services *code* or runtime services *data*? Code must obviously be remain encrypted (otherwise we cannot execute it in SEV). Runtime Services Data should also be mapped as encrypted (it is normal RAM that is not used for guest<->hypervisor exchange). > hence we end up accessing the flash as encrypted when OS requests to update > the variables. I don't understand the "hence" here; I don't see how the implication follows. runtime services code and data should be encrypted. Runtime MMIO should be un-encrypted. Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. ... Anyway, I think first we should go with the "check EFI_MEMORY_RUNTIME attribute before setting it", in FtwNotificationEvent(). > > A possible solution > - > To solve the issue, after QemuFlash is probed, I allocate an encrypted > buffer and initialize this buffer with the contents from the flash memory. > When SEV is enabled, we use newly allocated encrypted buffer in > FwInstance->FvBase instead of the original flash region. The idea is if > caller grabs the FwInstance->FvBase pointer and tries to access the > FvVolumeHeader then it should get the data from the encrypted buffer. > But if caller wants read/writes to/from the flash device then we internally > use the original "unencrypted" flash region to access the data. No, this is neither safe, nor a desirable design. Safety: all accesses (via both pointers and FVB protocol members) that higher-level drives *think* go to the pflash chip must *actually* go to the pflash chip. Design: it had taken us years to get rid of various memory-emulated fake variable stores. They *all* suck in one way or another, with various obscure UEFI spec incompatibilities and corner cases. A strictly pflash-based varstore is not what we should compromise on. > With this > patch, I have verified that OS is able to update the runtime variable and > FC-28 installer is successfully able to complete the installation process. > > If you all agree with approach then I can rework any feedbacks and remove > the rfc tag from the patch. If you have better suggestions then I am open > to explore those as well. I'd like to understand the following: (1) why does FtwNotificationEvent() set the EFI_MEMORY_RUNTIME attribute itself, for the pflash range? -- in my opinion, that belongs in the flash driver. (2) Whether Star and Eric agree with setting the EFI_MEMORY_RUNTIME attribute in FtwNotificationEvent() only if the attribute is not already present. (3) The implication that you describe, between runtime services/code being mapped encrypted, and restoring the C-bit failing. (4) Whether we should modify MarkMemoryRangeForRuntimeAccess() to
