Re: [edk2-devel] [PATCH 1/4] OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF

[James Bottomley <jejb@linux.ibm.com>]

On Mon, 2020-11-16 at 20:11 +0100, Laszlo Ersek wrote:
> On 11/12/20 01:13, James Bottomley wrote:
> > This commit represents the file copied from OvmfPkgX64 with minor
> > changes to change the build name.
> > 
> > This package will form the basis for adding Sev specific features.
> > Since everything must go into a single rom file for attestation,
> > the separated build of code and variables is eliminated.
> > 
> > Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> > ---
> >  OvmfPkg/AmdSev/AmdSevX64.dsc | 1024
> > ++++++++++++++++++++++++++++++++++
> >  OvmfPkg/AmdSev/AmdSevX64.fdf |  506 +++++++++++++++++
> >  2 files changed, 1530 insertions(+)
> >  create mode 100644 OvmfPkg/AmdSev/AmdSevX64.dsc
> >  create mode 100644 OvmfPkg/AmdSev/AmdSevX64.fdf
> > 
> > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> > b/OvmfPkg/AmdSev/AmdSevX64.dsc
> > new file mode 100644
> > index 0000000000..d1dfb8742f
> > --- /dev/null
> > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> > @@ -0,0 +1,1024 @@
> > +## @file
> > +#  EFI/Framework Open Virtual Machine Firmware (OVMF) platform for
> > SEV
> 
> (1) I suggest / request that we put "remote attestation" somewhere in
> the above file-top comment.

OK, will add

> > +#
> > +#  Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > reserved.<BR>
> > +#  (C) Copyright 2016 Hewlett Packard Enterprise Development
> > LP<BR>
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +##
> 
> (2) In every new file created in this series, please prepend an IBM
> Copyright Notice, to the original (C) notices (if any).

OK, I can add one.

> > +
> > +##################################################################
> > ##############
> > +#
> > +# Defines Section - statements that will be processed to create a
> > Makefile.
> > +#
> > +##################################################################
> > ##############
> > +[Defines]
> > +  PLATFORM_NAME                  = Ovmf
> > +  PLATFORM_GUID                  = 5a9e7754-d81b-49ea-85ad-
> > 69eaa7b1539b
> 
> (3) Please generate a new PLATFORM_GUID for this new platform with
> "uuidgen".

Sure ... I did do that the last time, I just forgot this time around.

> > +  PLATFORM_VERSION               = 0.1
> > +  DSC_SPECIFICATION              = 0x00010005
> > +  OUTPUT_DIRECTORY               = Build/AmdSev
> > +  SUPPORTED_ARCHITECTURES        = X64
> > +  BUILD_TARGETS                  = NOOPT|DEBUG|RELEASE
> > +  SKUID_IDENTIFIER               = DEFAULT
> > +  FLASH_DEFINITION               = OvmfPkg/AmdSev/AmdSevX64.fdf
> > +
> > +  #
> > +  # Defines for default states.  These can be changed on the
> > command line.
> > +  # -D FLAG=VALUE
> > +  #
> > +  DEFINE SECURE_BOOT_ENABLE      = FALSE
> > +  DEFINE SMM_REQUIRE             = FALSE
> 
> (4) SEV-ES doesn't support (to my knowledge) SMM, so we should strip
> everything dependent on SMM_REQUIRE being TRUE (DSC and FDF files
> both).
> 
> (5) Given that SMM cannot protect Secure Boot, SECURE_BOOT_ENABLE too
> should be assumed FALSE, and stuff dependent on SECURE_BOOT_ENABLE
> being
> TRUE should be stripped.

Yes, I believe neither of these is required.

> > +  DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> > +  DEFINE TPM_ENABLE              = FALSE
> > +  DEFINE TPM_CONFIG_ENABLE       = FALSE
> > +
> > +  #
> > +  # Network definition
> > +  #
> > +  DEFINE NETWORK_TLS_ENABLE             = FALSE
> > +  DEFINE NETWORK_IP6_ENABLE             = FALSE
> > +  DEFINE NETWORK_HTTP_BOOT_ENABLE       = FALSE
> > +  DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = TRUE
> 
> (6) My understanding is that netboot with this platform is never
> desired. If that's the case, then please remove:
> - all the NETWORK_* flags,
> - the dependent DSC/FDF snippets,
> - the gEfiNetworkPkgTokenSpaceGuid.* PCD defaults,
> - and (in particular) all !include directives that refer to
> NetworkPkg/*

I don't think we'll ever have a network dependency, no.

> My goal with the above trimming is two-fold:
> 
> - avoid an implication for platform builders that they can
> meaningfully
> tweak the -D flags for this platform,
> 
> - cut down on the size of the new DSC/FDF files (given that the above
> fruits seem to hang low).

Will do.

Regards,

James