Re: [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled

[Brijesh Singh <brijesh.singh@amd.com>]

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 FFC00000 - FFFFFFFF
>> 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),
                    &BaseAddress
                    );

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