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 <brijesh.si...@amd.com>; edk2-devel@lists.01.org
> Cc: Tom Lendacky <thomas.lenda...@amd.com>; Dong, Eric <eric.d...@intel.com>;
> Zeng, Star <star.z...@intel.com>; Justen, Jordan L <jordan.l.jus...@intel.com>
> 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 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 install the
> range into GCD as MMIO. (I feel *very* uncomfortable about this, however; the
> current code has existed as-is for years, and regressions look very risky.)
>
> My strong preference would be a patch for (2).
>
> Thanks,
> Laszlo
>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
>> ---
>> .../FvbServicesRuntimeDxe.inf | 1 +
>> .../FwBlockService.c | 37
>> +++++++++++++++++++---
>> 2 files changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git
>> a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>> index d7b4ec06c4e6..6bb5c2093790 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>> @@ -54,6 +54,7 @@ [LibraryClasses]
>> DevicePathLib
>> DxeServicesTableLib
>> MemoryAllocationLib
>> + MemEncryptSevLib
>> PcdLib
>> UefiBootServicesTableLib
>> UefiDriverEntryPoint
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> index 558b395dff4a..e82b4ff70961 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -36,6 +36,7 @@
>> #include <Library/DxeServicesTableLib.h> #include
>> <Library/MemoryAllocationLib.h> #include
>> <Library/UefiBootServicesTableLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>
>> #include "FwBlockService.h"
>> #include "QemuFlash.h"
>> @@ -966,6 +967,7 @@ FvbInitialize (
>> UINTN Length;
>> UINTN NumOfBlocks;
>> RETURN_STATUS PcdStatus;
>> + EFI_PHYSICAL_ADDRESS CryptedAddress;
>>
>> if (EFI_ERROR (QemuFlashInitialize ())) {
>> //
>> @@ -986,6 +988,24 @@ FvbInitialize (
>> BaseAddress = (UINTN) PcdGet32 (PcdOvmfFdBaseAddress);
>> Length = PcdGet32 (PcdOvmfFirmwareFdSize);
>>
>> + //
>> + // When SEV is enabled, allocate a encrypted buffer which will
>> + contain a // encrypted copy of the Flash image.
>> + //
>> + if (MemEncryptSevIsEnabled ()) {
>> + Status = gBS->AllocatePages (
>> + AllocateAnyPages,
>> + EfiRuntimeServicesData,
>> + EFI_SIZE_TO_PAGES(PcdGet32 (PcdOvmfFirmwareFdSize)),
>> + &CryptedAddress
>> + );
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + CopyMem((VOID *)CryptedAddress, (VOID *)BaseAddress, Length);
>> +
>> + BaseAddress = CryptedAddress;
>> + }
>> +
>> Status = InitializeVariableFvHeader ();
>> if (EFI_ERROR (Status)) {
>> DEBUG ((EFI_D_INFO,
>> @@ -1091,24 +1111,33 @@ FvbInitialize (
>> //
>> InstallProtocolInterfaces (FvbDevice);
>>
>> - MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
>> + MarkMemoryRangeForRuntimeAccess (
>> + (UINTN) PcdGet32 (PcdOvmfFdBaseAddress),
>> + Length
>> + );
>>
>> //
>> // Set several PCD values to point to flash
>> //
>> PcdStatus = PcdSet64S (
>> PcdFlashNvStorageVariableBase64,
>> - (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
>> + BaseAddress
>> );
>> ASSERT_RETURN_ERROR (PcdStatus);
>> PcdStatus = PcdSet32S (
>> PcdFlashNvStorageFtwWorkingBase,
>> - PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
>> + BaseAddress +
>> + PcdGet32(PcdFlashNvStorageVariableSize) +
>> + PcdGet32(PcdOvmfFlashNvStorageEventLogSize)
>> );
>> +
>> ASSERT_RETURN_ERROR (PcdStatus);
>> PcdStatus = PcdSet32S (
>> PcdFlashNvStorageFtwSpareBase,
>> - PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
>> + BaseAddress +
>> + PcdGet32(PcdFlashNvStorageVariableSize) +
>> + PcdGet32(PcdOvmfFlashNvStorageEventLogSize) +
>> + PcdGet32(PcdFlashNvStorageFtwWorkingSize)
>> );
>> ASSERT_RETURN_ERROR (PcdStatus);
>>
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
