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 <ler...@redhat.com>; edk2-devel@lists.01.org Cc: Tom Lendacky <thomas.lenda...@amd.com>; brijesh.si...@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/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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
