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

["Zeng, Star" <star.zeng@intel.com>]

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-bounces@lists.01.org] On Behalf Of Brijesh Singh
Sent: Thursday, June 28, 2018 1:50 AM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; brijesh.singh@amd.com; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>
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 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel