From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.1036.1605571967320545942 for ; Mon, 16 Nov 2020 16:12:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZsuVaV4m; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605571966; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oGVVM9nEkI/vqJCWdBWVwkAU1kmEFrfuG9FCW6wQZ1U=; b=ZsuVaV4m6kYShZ43qxYg1CLrKf6HFkyKstjD1klX+FVZF++NOCzzjZfNQfrnTYBMxhmdl/ YXPIdx9MhKzdQ3PIanyXgCLYksVSf4nEEf14vY6y/FwoBystBX7QAdnkdrqEMr3D9QBJzl emE3Lq6beT02tZwkgGS403PMth+stKg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-510-fYgeoWjxPPiOOztpYRjfhQ-1; Mon, 16 Nov 2020 19:12:44 -0500 X-MC-Unique: fYgeoWjxPPiOOztpYRjfhQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 667C757241; Tue, 17 Nov 2020 00:12:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-190.ams2.redhat.com [10.36.112.190]) by smtp.corp.redhat.com (Postfix) with ESMTP id 732E15D9D2; Tue, 17 Nov 2020 00:12:39 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 4/4] OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table To: devel@edk2.groups.io, jejb@linux.ibm.com Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" References: <20201112001316.11341-1-jejb@linux.ibm.com> <20201112001316.11341-5-jejb@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <91f0944b-4cd4-1e27-016f-489ffa8e8ba1@redhat.com> Date: Tue, 17 Nov 2020 01:12:38 +0100 MIME-Version: 1.0 In-Reply-To: <20201112001316.11341-5-jejb@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/12/20 01:13, James Bottomley wrote: > This is to allow the boot loader (grub) to pick up the secret area. > The Configuration Table simply points to the base and size (in > physical memory) and this area is covered by a Boot time HOB, meaning > that the secret will be freed after ExitBootServices, by which time it > should be consumed anyway. > > Signed-off-by: James Bottomley > --- > OvmfPkg/AmdSev/AmdSevX64.dsc | 3 ++ > OvmfPkg/AmdSev/AmdSevX64.fdf | 3 ++ > .../SevLaunchSecret/SecretDxe/SecretDxe.inf | 38 +++++++++++++++ > .../SevLaunchSecret/SecretPei/SecretPei.inf | 46 +++++++++++++++++++ > .../SevLaunchSecret/SecretDxe/SecretDxe.c | 29 ++++++++++++ > .../SevLaunchSecret/SecretPei/SecretPei.c | 26 +++++++++++ > 6 files changed, 145 insertions(+) > create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf > create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf > create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c > create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc > index 7d3663150e..eb8cc9d60a 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.dsc > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc > @@ -698,6 +698,7 @@ > OvmfPkg/SmmAccess/SmmAccessPei.inf > !endif > UefiCpuPkg/CpuMpPei/CpuMpPei.inf > + OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -1007,6 +1008,8 @@ > } > !endif > > + OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf > + > # > # TPM support > # > diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf > index 1fd38b3fe2..65ee4d993b 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.fdf > +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf > @@ -146,6 +146,7 @@ INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf > INF OvmfPkg/SmmAccess/SmmAccessPei.inf > !endif > INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf > +INF OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -290,6 +291,8 @@ INF ShellPkg/Application/Shell/Shell.inf > > INF MdeModulePkg/Logo/LogoDxe.inf > > +INF OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf > + > # > # Network modules > # (1) Please split the SecretDxe-related hunks and new files to a separate patch. (1.a) Patch#4 should be called OvmfPkg/AmdSev: assign and protect the Sev Secret area It should contain the PEI phase-related changes. (1.b) Patch#5 should inherit the present subject ("OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table"), and should contain the DXE phase-related hunks and new files. I'll comment on patch#5 more, below. (2) In both the DSC and the FDF files, please place the OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf just above OvmfPkg/AmdSev/Grub/Grub.inf (Not a functional difference, it just improves consistency between the DSC and the FDF, plus this placement appears quite logical too.) (3) I suggest unnesting both SecretPei and SecretDxe from the SevLaunchSecret subdirectory, and removing that subdirectory. To me the following pathnames seem fine: - OvmfPkg/AmdSev/SecretPei/ - OvmfPkg/AmdSev/SecretDxe/ > diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf > new file mode 100644 > index 0000000000..b154dcc74e > --- /dev/null > +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf > @@ -0,0 +1,46 @@ > +## @file > +# PEI support for SEV Secrets > +# > +# Copyright (C) 2020 James Bottomley, IBM Corporation. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = SecretPei > + FILE_GUID = 45260dde-0c3c-4b41-a226-ef3803fac7d4 > + MODULE_TYPE = PEIM > + VERSION_STRING = 1.0 > + ENTRY_POINT = InitializeSecretPei > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 EBC > +# (4) Please either drop VALID_ARCHITECTURES, or restrict it to X64 only. > + > +[Sources] > + SecretPei.c > + > +[Packages] > + OvmfPkg/OvmfPkg.dec > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec (5) MdeModulePkg seems unnecessary > + > +[LibraryClasses] > + BaseLib > + DebugLib > + HobLib > + PeiServicesLib > + PeiServicesTablePointerLib > + PeimEntryPoint > + PcdLib (6) only HobLib, PcdLib, and PeimEntryPoint appear necessary > + > +[FixedPcd] > + gSevLaunchSecretGuid.PcdSevLaunchSecretBase > + gSevLaunchSecretGuid.PcdSevLaunchSecretSize > + > +[Depex] > + TRUE > diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c > new file mode 100644 > index 0000000000..16b49792ad > --- /dev/null > +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c > @@ -0,0 +1,26 @@ > +/** @file > + SEV Secret boot time HOB placement > + > + Copyright (C) 2020 James Bottomley, IBM Corporation. > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > +#include > +#include > +#include > +#include > +#include (7) From Library/, only HobLib and PcdLib look required. After removing the other lib class headers, you may have to include separately, for EfiBootServicesData. > + > +EFI_STATUS > +EFIAPI > +InitializeSecretPei ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + BuildMemoryAllocationHob ( > + PcdGet32 (PcdSevLaunchSecretBase), > + PcdGet32 (PcdSevLaunchSecretSize), > + EfiBootServicesData); (8) Please break the closing paren to a new line: BuildMemoryAllocationHob ( PcdGet32 (PcdSevLaunchSecretBase), PcdGet32 (PcdSevLaunchSecretSize), EfiBootServicesData ); (Note that the indentation of the closing paren is not a mistake, it is intentional.) > + > + return EFI_SUCCESS; > +} > > diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf > new file mode 100644 > index 0000000000..085162e5c4 > --- /dev/null > +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf > @@ -0,0 +1,38 @@ > +## @file > +# Sev Secret configuration Table installer > +# > +# Copyright (C) 2020 James Bottomley, IBM Corporation. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = SecretDxe > + FILE_GUID = 6e2b9619-8810-4e9d-a177-d432bb9abeda > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = InitializeSecretDxe > + > +[Sources] > + SecretDxe.c > + > +[Packages] > + OvmfPkg/OvmfPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + UefiLib (9) UefiLib looks superfluous > + > +[Guids] > + gSevLaunchSecretGuid > + > +[FixedPcd] > + gSevLaunchSecretGuid.PcdSevLaunchSecretBase > + gSevLaunchSecretGuid.PcdSevLaunchSecretSize > + > +[Depex] > + TRUE > diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c > new file mode 100644 > index 0000000000..b40bbe1eb9 > --- /dev/null > +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c > @@ -0,0 +1,29 @@ > +/** @file > + SEV Secret configuration table constructor > + > + Copyright (C) 2020 James Bottomley, IBM Corporation. > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > +#include > +#include (10) UefiLib looks superfluous. > +#include (11) Entry point libraries must be listed in the INF files' [LibraryClasses] sections, but should not be #included in the C source files. > +#include > + > +struct { > + UINT32 base; > + UINT32 size; > +} secretDxeTable = { > + FixedPcdGet32(PcdSevLaunchSecretBase), > + FixedPcdGet32(PcdSevLaunchSecretSize), > +}; > + (12) This is an interface between (theoretically) independent applications, so we should place the structure definition in a dedicated header file. (12.a) Please move the following hunk, from patch#3: > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 3fbf7a0ee1a4..b00f08341713 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -117,6 +117,7 @@ [Guids] > gLinuxEfiInitrdMediaGuid = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}} > gQemuKernelLoaderFsMediaGuid = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}} > gGrubFileGuid = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}} > + gSevLaunchSecretGuid = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}} > > [Ppis] > # PPI whose presence in the PPI database signals that the TPM base address to patch#5 (i.e., the patch that's going to introduce SecretDxe). (12.b) In patch #5, please create another new header file: OvmfPkg/Include/Guid/SevLaunchSecret.h with the following contents (fix up the (C) notice): > /** @file > UEFI Configuration Table for exposing the SEV Launch Secret location to UEFI > applications (boot loaders). > > Copyright (C) > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > > #ifndef SEV_LAUNCH_SECRET_H_ > #define SEV_LAUNCH_SECRET_H_ > > #include > > #define SEV_LAUNCH_SECRET_GUID \ > { 0xadf956ad, \ > 0xe98c, \ > 0x484c, \ > { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47 }, \ > } > > typedef struct { > UINT32 Base; > UINT32 Size; > } SEV_LAUNCH_SECRET_LOCATION; > > extern EFI_GUID gSevLaunchSecretGuid; > > #endif // SEV_LAUNCH_SECRET_H_ (12.c) In the "SecretDxe.c" file, please use the following syntax, for defining the table: > #include > > STATIC CONST SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = { > FixedPcdGet32 (PcdSevLaunchSecretBase), > FixedPcdGet32 (PcdSevLaunchSecretSize), > }; (12.d) Note the space character just after "FixedPcdGet32", and the "m" prefix in the global variable's name. Back to your patch: On 11/12/20 01:13, James Bottomley wrote: > +EFI_STATUS > +EFIAPI > +InitializeSecretDxe( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid, > + &secretDxeTable); > +} (13) There are two styles to indent this idiomatically: (13.a) the official edk2 one: return gBS->InstallConfigurationTable ( &gSevLaunchSecretGuid, &mSecretDxeTable ); - each argument *and* the closing paren on a new line, - each argument *and* the closing paren indented 2 spaces relative to "InstallConfigurationTable" (not relative to "gBS") (13.b) or the "condensed" style: return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid, &mSecretDxeTable); The only difference from (13.a) is that a new source line is only started when we'd exceed the recommended line length (80). Thanks, Laszlo