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 8BE95210F2544 for ; Tue, 3 Jul 2018 08:08:34 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AE61840267D0; Tue, 3 Jul 2018 15:08:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-222.rdu2.redhat.com [10.10.120.222]) by smtp.corp.redhat.com (Postfix) with ESMTP id 786621117629; Tue, 3 Jul 2018 15:08:32 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Justen Jordan L References: <1530587467-19571-1-git-send-email-brijesh.singh@amd.com> <1530587467-19571-2-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Tue, 3 Jul 2018 17:08:31 +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: <1530587467-19571-2-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 03 Jul 2018 15:08:33 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 03 Jul 2018 15:08:33 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Runtime Data as MMIO when SEV is active 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: Tue, 03 Jul 2018 15:08:34 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Brijesh, On 07/03/18 05:11, Brijesh Singh wrote: > When SEV is active, the flash memory range is mapped as unencrypted by > AmdSevDxe. Mark the flash memory range with EfiGcdMemoryTypeMemoryMappedIo > so that OS maps this memory range as unencrypted. > > Cc: Justen Jordan L > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > > Hi Laszlo, > > I have tried marking flash memory range as MMIO for non SEV guest, and > everything seems to be working fine but I was not sure if we will break > something else in non SEV case. Because of this I have created a new > routine which marks the range as MMIO only when SEV is active. > > .../FvbServicesRuntimeDxe.inf | 1 + > .../FwBlockService.c | 69 +++++++++++++++++++++- > 2 files changed, 69 insertions(+), 1 deletion(-) Here's what I suggest: (1) Please send the patch for MdeModulePkg/VariableDxe separately, with the updates that Star requests. We should commit that patch first (and separately), so that we can collaborate on the OvmfPkg patches without disturbing Star with further reposts. (2) For OvmfPkg, I'd like you to split this change in three parts: (2a) In the first patch, please switch MarkMemoryRangeForRuntimeAccess() from the runtime memory marking to the runtime MMIO marking. This should extend up to and including the SetMemorySpaceAttributes() call. Also, rename MarkMemoryRangeForRuntimeAccess() to MarkIoMemoryRangeForRuntimeAccess(). In my opinion, this is something we should do regardless of SEV. (2b) In the second patch, please declare MarkIoMemoryRangeForRuntimeAccess() at the end of "FwBlockService.h", and move the implementation to "FwBlockServiceDxe.c". For the SMM build, add an empty stub to "FwBlockServiceSmm.c". This is because the "MMIO+runtime" marking stands for "a runtime DXE driver or service is using this address range". However, in the SMM build, only an SMM driver is using the address range; I don't think we need to expose the range in the UEFI memmap at all. (2c) In the third patch, modify the implementation in "FwBlockServiceDxe.c", so that the SEV case is handled at the end of MarkIoMemoryRangeForRuntimeAccess(). (By clearing the C-bit again.) The end result *should* be that: - in the non-SMM build, - we expose the area as runtime MMIO in the UEFI memmap, - we handle SEV explicitly, - and the variable DXE driver leaves the attributes alone, - in the SMM build, - we don't expose the area at all, - we clear the C-bit *earlier* (in QemuFlashBeforeProbe()), - the variable SMM driver leaves the GCD / UEFI maps alone. Obviously we'll have to regression test both builds with a number of guest OSes; I will help with that (I can test going back to RHEL-6 and Windows-7). I have two more comments below: > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > index d7b4ec06c4e6..1af675852c86 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > @@ -58,6 +58,7 @@ [LibraryClasses] > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiRuntimeLib > + MemEncryptSevLib > > [Guids] > gEfiEventVirtualAddressChangeGuid # ALWAYS_CONSUMED > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > index 558b395dff4a..3aa21466556a 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > @@ -36,6 +36,7 @@ > #include > #include > #include > +#include > > #include "FwBlockService.h" > #include "QemuFlash.h" > @@ -867,6 +868,64 @@ MarkMemoryRangeForRuntimeAccess ( > > STATIC > EFI_STATUS > +SevMarkMemoryRangeForRuntimeAccess ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN Length > + ) > +{ > + EFI_STATUS Status; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; > + > + // > + // Mark flash region as runtime memory > + // > + Status = gDS->RemoveMemorySpace ( > + BaseAddress, > + Length > + ); > + > + Status = gDS->AddMemorySpace ( > + EfiGcdMemoryTypeMemoryMappedIo, > + BaseAddress, > + Length, > + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > + ); > + ASSERT_EFI_ERROR (Status); > + > + Status = gDS->AllocateMemorySpace ( > + AllocateAddress, > + EfiGcdMemoryTypeMemoryMappedIo, > + 0, > + EFI_SIZE_TO_PAGES (Length), (3) This seems wrong; AllocateMemorySpace() takes a number of bytes. (I realize you pasted this verbatim from an earlier suggestion that I made; sorry about that. :) ) > + &BaseAddress, > + gImageHandle, > + NULL > + ); > + ASSERT_EFI_ERROR (Status); > + > + Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor); > + ASSERT_EFI_ERROR (Status); > + > + Status = gDS->SetMemorySpaceAttributes ( > + BaseAddress, > + Length, > + GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME > + ); > + ASSERT_EFI_ERROR (Status); > + > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + BaseAddress, > + EFI_SIZE_TO_PAGES (Length), > + FALSE > + ); > + ASSERT_EFI_ERROR (Status); (4) Please add a comment before MemEncryptSevClearPageEncMask(), explaining that the call is necessary because SetMemorySpaceAttributes() re-sets the C-bit, and we need to undo that. Thanks! Laszlo > + > + return Status; > +} > + > +STATIC > +EFI_STATUS > InitializeVariableFvHeader ( > VOID > ) > @@ -1091,7 +1150,15 @@ FvbInitialize ( > // > InstallProtocolInterfaces (FvbDevice); > > - MarkMemoryRangeForRuntimeAccess (BaseAddress, Length); > + // > + // When SEV is enabled, mark the flash region as MMIO to hint the OS that > + // the memory range need to be mapped as unencrypted. > + // > + if (MemEncryptSevIsEnabled()) { > + SevMarkMemoryRangeForRuntimeAccess (BaseAddress, Length); > + } else { > + MarkMemoryRangeForRuntimeAccess (BaseAddress, Length); > + } > > // > // Set several PCD values to point to flash >