From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8DC08202E53D9 for ; Thu, 28 Jun 2018 06:21:06 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C71C887A80; Thu, 28 Jun 2018 13:21:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-10.rdu2.redhat.com [10.10.120.10]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6236D2026D69; Thu, 28 Jun 2018 13:21:04 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Star Zeng , Eric Dong , "Jordan Justen (Intel address)" References: <1530042365-9979-1-git-send-email-brijesh.singh@amd.com> <272c4a0f-fcc1-2899-e31d-a3207feb51ed@redhat.com> <2a662245-bb03-e742-1403-4d0a47bffda7@amd.com> Message-ID: Date: Thu, 28 Jun 2018 15:21:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 28 Jun 2018 13:21:05 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 28 Jun 2018 13:21:05 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Jun 2018 13:21:06 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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