On 06/28/18 14:57, Laszlo Ersek wrote: > On 06/27/18 19:49, Brijesh Singh wrote: >> >> >> 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. > > Hmmm. Indeed, memory space added to GCD need not immediately show up in > the UEFI memory map, for the UEFI memory allocation services to allocate > from. IIRC, the PI spec says that *system memory* added like this *may* > show up immediately: > > """ > 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. > """ > > and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at > all. > > So, can you replace the AllocatePages call with the following: > > Status = gDS->AllocateMemorySpace ( > EfiGcdAllocateAddress, > EfiGcdMemoryTypeMemoryMappedIo, > 0, // Alignment > EFI_SIZE_TO_PAGES (Length), > &BaseAddress, > gImageHandle, > NULL // DeviceHandle > ); > >>> 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 > > More correctly: SMM drivers use, at runtime as well, the page table in > SMRAM that was created by the firmware. > >> hence after fixing FtwNotificationEvent(..) we should be >> good. > > No, that's not precise -- FtwNotificationEvent() is not used in SMM *at > all*.
Sigh, I misunderstood you. You meant, "after fixing DXE, we should be good, because the the issue only affects DXE". I mistook your statement as "after we fix DXE, both DXE and SMM will be OK, because the fix affects both DXE and SMM". That was not a correct statement (because the fix only affects DXE; SMM is unaffected and needs no fix), which is why I attempted to correct it. Of course, you never *said* that statement. :) Sorry about the noise! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
