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

[Laszlo Ersek <lersek@redhat.com>]

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.

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