From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.158.5]) by mx.groups.io with SMTP id smtpd.web09.48646.1606199910768146416 for ; Mon, 23 Nov 2020 22:38:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=ZwKAd56u; spf=pass (domain: linux.ibm.com, ip: 148.163.158.5, mailfrom: jejb@linux.ibm.com) Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 0AO63NcM055740; Tue, 24 Nov 2020 01:38:28 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : subject : from : reply-to : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding; s=pp1; bh=y3WKzwIh5WJQtfegG6RCH6kB/h25cPnbeSYvUwkgVhE=; b=ZwKAd56ukAo0h3Ha2Lsh0KNzxyT32N0zUIKY/j2c2ds52GLS2sfdV+sNI3Uk6OoN5T4L yAcZFmL4Xu94LwAxUbAdhMS1C8hnqs53L+9S0fckic+I+z+WVWYwn6IjoxoPxgAycY8g JpG+mKaGUhmtxe1Ky1fCW9HFe6oZoymet2DnmohRjAmUAwDIJUNcufL8ziCXX4D2e9jY 7JRRrDIeSm3NZ5AQNrVNcioSpdQUV0lrnb54EMNNh9NUyUd+1/6sQIP2EqzWNW8pA8d4 gUcF1+sD/sL04meXO9Uyz+UU5rAUvmn9cfJ1LGlbnslD7ZFsciS0CyJ34fLSpnj6IcMG Eg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 350uq2hw7v-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Nov 2020 01:38:28 -0500 Received: from m0098414.ppops.net (m0098414.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 0AO6cJ4U180195; Tue, 24 Nov 2020 01:38:28 -0500 Received: from ppma04wdc.us.ibm.com (1a.90.2fa9.ip4.static.sl-reverse.com [169.47.144.26]) by mx0b-001b2d01.pphosted.com with ESMTP id 350uq2hw7j-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Nov 2020 01:38:28 -0500 Received: from pps.filterd (ppma04wdc.us.ibm.com [127.0.0.1]) by ppma04wdc.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 0AO6WcGi013024; Tue, 24 Nov 2020 06:38:27 GMT Received: from b03cxnp07029.gho.boulder.ibm.com (b03cxnp07029.gho.boulder.ibm.com [9.17.130.16]) by ppma04wdc.us.ibm.com with ESMTP id 34xth8w464-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Nov 2020 06:38:27 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 0AO6cOpl61145552 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Nov 2020 06:38:24 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 68D027805C; Tue, 24 Nov 2020 06:38:24 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3DBD178064; Tue, 24 Nov 2020 06:38:22 +0000 (GMT) Received: from jarvis.int.hansenpartnership.com (unknown [9.85.194.234]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Tue, 24 Nov 2020 06:38:21 +0000 (GMT) Message-ID: <06b9425507ab8c1b35d377cf9bba155b0cc44147.camel@linux.ibm.com> Subject: Re: [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package From: "James Bottomley" Reply-To: jejb@linux.ibm.com To: Laszlo Ersek , devel@edk2.groups.io 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" Date: Mon, 23 Nov 2020 22:38:20 -0800 In-Reply-To: <28e99174-79b3-e805-b977-5fed0071a702@redhat.com> References: <20201120184521.19437-1-jejb@linux.ibm.com> <20201120184521.19437-3-jejb@linux.ibm.com> <28e99174-79b3-e805-b977-5fed0071a702@redhat.com> User-Agent: Evolution 3.34.4 MIME-Version: 1.0 X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312,18.0.737 definitions=2020-11-24_01:2020-11-24,2020-11-23 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 clxscore=1015 bulkscore=0 suspectscore=0 spamscore=0 priorityscore=1501 malwarescore=0 adultscore=0 mlxscore=0 lowpriorityscore=0 impostorscore=0 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2011240036 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On Mon, 2020-11-23 at 22:08 +0100, Laszlo Ersek wrote: [...] > (1) typo -- should be "Bootloader", I believe (apologies for missing > it in v1) fixed. [...] > > +## > > +# Note: The version of grub.efi this picks up can be generated by > > +# grub.sh which must be specified as a PREBUILD in the .dsc file > > or > > +# you can simply move a precompiled grub into here and not do the > > +# PREBUILD) > > (2) This part of the patch looks unchanged, and I asked for more > explanation in [a](3) -- "Can you elaborate how to skip PREBUILD"? > > The comment in the patch remains the same, and (AFAICT) I have not > received a response within the v1 thread either. (Instead you stated > "I did everything except [...].) > > So now I'm not sure... Did you miss [a](3), or did you decide it was > not important enough to discuss / address? Actually, the change was to make it true. In the previous patch grub.efi was always rebuilt in spite of what the comment in Grub.inf implied. Now it's only rebuilt if grub.efi is non-existent or older than either grub.sh or grub.cfg. I've expanded the comment. I see you picked this up in 10 below. > > +## > > +[Binaries] > > + PE32|grub.efi|* > > + > > diff --git > > a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLib > > Grub.inf > > b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLib > > Grub.inf > > new file mode 100644 > > index 000000000000..a997d7586e6a > > --- /dev/null > > +++ > > b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLib > > Grub.inf > > @@ -0,0 +1,79 @@ > > +## @file > > +# Platform BDS customizations library. > > +# > > +# Copyright (c) 2007 - 2019, Intel Corporation. All rights > > reserved.
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > +## > > (3) In the v1 review thread, under patch #1, I requested: "In every > new file created in this series, please prepend an IBM Copyright > Notice, to the original (C) notices (if any)." > > https://edk2.groups.io/g/devel/message/67615 > > https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00775.html > > This is a new file, but it still has no IBM (C) notice. Slipped through the cracks: I've added it. > > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > + BASE_NAME = PlatformBootManagerLibGrub > > + FILE_GUID = 3a8f8431-f0c9-4c95-8a1d- > > 04445c582d4e > > + MODULE_TYPE = DXE_DRIVER > > + VERSION_STRING = 1.0 > > + LIBRARY_CLASS = > > PlatformBootManagerLib|DXE_DRIVER > > + > > +# > > +# The following information is for reference only and not required > > by the build tools. > > +# > > +# VALID_ARCHITECTURES = X64 > > +# > > + > > +[Sources] > > + BdsPlatform.c > > + PlatformData.c > > + BdsPlatform.h > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + SourceLevelDebugPkg/SourceLevelDebugPkg.dec > > + OvmfPkg/OvmfPkg.dec > > + SecurityPkg/SecurityPkg.dec > > + ShellPkg/ShellPkg.dec > > + > > +[LibraryClasses] > > + BaseLib > > + MemoryAllocationLib > > + UefiBootServicesTableLib > > + UefiRuntimeServicesTableLib > > + BaseMemoryLib > > + DebugLib > > + PcdLib > > + UefiBootManagerLib > > + BootLogoLib > > + DevicePathLib > > + PciLib > > + ReportStatusCodeLib > > + UefiLib > > + PlatformBmPrintScLib > > + Tcg2PhysicalPresenceLib > > + XenPlatformLib > > (4) I suggested removing XenPlatformLib too, in [a](4). > > Did you run into build problems with that? > > Replacing the XenDetected() calls in "BdsPlatform.c" with constant > FALSE, and simplifying the resultant code, should suffice. No I just missed this part pursuing another set of stripping. > > + > > +[Pcd] > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable > > (5) Thanks for removing PcdEmuVariableEvent. > > But, PcdOvmfFlashVariablesEnable is just as superfluous; the INF file > now lists it without any references to the PCD in the code. OK, removed. > In [a](5) I requested the removal of everything in this INF file that > was not required for the desired semantics (i.e., for unconditionally > booting the builtin GRUB binary). > > So again I'm unsure if you missed my feedback, or thought it was not > important. (I didn't get a request for clarification either.) > > Keeping INF files minimal is relevant for future contributions. We > frequently need to determine the set of modules that depend on a > particular PCD. If some INF files list PCDs unjustifiedly, then the > affected module set may appear larger than it really is. > > This applies to all sections of the INF file, not just [Pcd]. I did try stripping quite a lot, but then it wouldn't boot. It seems that the PCI devices are needed for grub to find the encrypted volume, so I put most of it back again. Is there some way of identifying superfluous pieces so I can detect if I put too much back? [...] > > diff --git > > a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h > > b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h > > new file mode 100644 > > index 000000000000..a7fc4dbc3c1f > > --- /dev/null > > +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h > > @@ -0,0 +1,175 @@ > > +/** @file > > + Platform BDS customizations include file. > > + > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights > > reserved.
> > + SPDX-License-Identifier: BSD-2-Clause-Patent > > (6) Same as (3) -- missing IBM (C) on new file Sorry, added. > > > diff --git > > a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c > > b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c > > new file mode 100644 > > index 000000000000..4fb2f904a10e > > --- /dev/null > > +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c > > @@ -0,0 +1,1483 @@ > > +/** @file > > + Platform BDS customizations. > > + > > + Copyright (c) 2004 - 2019, Intel Corporation. All rights > > reserved.
> > + SPDX-License-Identifier: BSD-2-Clause-Patent > > (7) same as (3) -- missing IBM (C) on new file Sorry, added. > > +/** > > + The function is called when no boot option could be launched, > > + including platform recovery options and options pointing to > > applications > > + built into firmware volumes. > > + > > + If this function returns, BDS attempts to enter an infinite > > loop. > > +**/ > > +VOID > > +EFIAPI > > +PlatformBootManagerUnableToBoot ( > > + VOID > > + ) > > +{ > > + // > > + // If we get here something failed about the grub boot but since > > + // We're privy to the secret we must panic and not retry or loop > > + // > > + CpuDeadLoop (); > > +} > > (8) In [a](7) I meant that both ASSERT() and CpuDeadLoop() should be > called. In DEBUG and NOOPT builds, the ASSERT() fires, produces a > somewhat helpful debug message, and the CpuDeadLoop() is not reached > (ASSERT() may call CpuDeadLoop() internally, or raise an exception). > In RELEASE builds, the message is not produced, but we still won't > proceed past the CpuDeadLoop(). > > Sorry about not being clear enough. OK, I put both in. > > diff --git > > a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c > > b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c > > new file mode 100644 > > index 000000000000..2858c3dfd5ca > > --- /dev/null > > +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c > > @@ -0,0 +1,213 @@ > > +/** @file > > + Defined the platform specific device path which will be used by > > + platform Bbd to perform the platform policy connect. > > + > > + Copyright (c) 2004 - 2017, Intel Corporation. All rights > > reserved.
> > + SPDX-License-Identifier: BSD-2-Clause-Patent > > (9) same as (3) -- missing IBM (C) on new file Sorry, added. > > diff --git a/OvmfPkg/AmdSev/Grub/grub.sh > > b/OvmfPkg/AmdSev/Grub/grub.sh > > new file mode 100644 > > index 000000000000..fd28dc6ab274 > > --- /dev/null > > +++ b/OvmfPkg/AmdSev/Grub/grub.sh > > @@ -0,0 +1,92 @@ > > +## @file > > +# Build a version of grub capable of decrypting a luks volume > > with a SEV > > +# Supplied secret > > +# > > +# Copyright (C) 2020 James Bottomley, IBM Corporation. > > +# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > +## > > + > > +set -e > > +remove_efi=1 > > + > > +cleanup() { > > + # remove the intermediates > > + for f in disk.fat grub-bootstrap.cfg; do > > + rm -f "${basedir}/$f" > > + done > > + if [ $remove_efi -eq 1 ]; then > > + rm -f "${basedir}/grub.efi" > > + fi > > +} > > (10) These two "rm -f" commands don't use the separator "--", unlike > the "rm -f" command below (which has been updated, thanks for that). Hm, yes, added. > > + > > +trap cleanup EXIT > > + > > +GRUB_MODULES=" > > + part_msdos > > + part_gpt > > + cryptodisk > > + luks > > + gcry_rijndael > > + gcry_sha256 > > + ext2 > > + btrfs > > + xfs > > + fat > > + configfile > > + memdisk > > + sleep > > + normal > > + echo > > + test > > + regexp > > + linux > > + linuxefi > > + reboot > > + sevsecret > > + " > > +basedir=$(dirname -- "$0") > > + > > +# don't run a build if grub.efi exists and is newer than the > > config files > > +if [ -e "${basedir}/grub.efi" -a \ > > + "${basedir}/grub.efi" -nt "${basedir}/grub.cfg" -a \ > > + "${basedir}/grub.efi" -nt "${basedir}/grub.sh" ]; then > > + remove_efi=0 > > + echo "preserving existing grub.efi" > > arguably this should go to stderr as well, but I don't insist Sure, added. > > + exit 0 > > +fi > > > > Ah, OK. This sort of addresses my question (2). Not entirely -- the > comment at (2) implies the user is somehow responsible for skipping > PREBUILD. While in reality, PREBUILD will be launched, it will just > exit early. Right, but I expanded the comment to describe the conditions. The idea is the distro could checkout edk2 then copy their own grub.efi in and it will build. > (11) Independently, "-a" is considered obsolescent, per > < > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16> > ;. > > Anyway, feel free to ignore. It's no trouble to use the && format. > > +## > > +# different distributions have different names for grub-mkimage, > > so > > +# search all the known ones > > +## > > +for b in grub2-mkimage grub-mkimage; do > > + if which "$b" > /dev/null 2>&1; then > > + mkimage="$b" > > + break; > > (12) "muscle memory" semicolon after "break" :) Um, yes .... > (13) the indentation seems strange (Linux kernel style?); please > don't use hardware tabs. (Hmmm, applies to other parts of this script > too.) OK, I got emacs to untabify it. I did try converting it to DOS format like the rest of edk2 but bash really doesn't like that: /home/jejb/git/edk2/OvmfPkg/AmdSev/Grub/grub.sh: line 10: $'\r': command not found > > + fi > > +done > > +if [ -z "$mkimage" ]; then > > + echo "Can't find grub mkimage" >&2 > > + exit 1 > > +fi > > (14) The variable "mkimage" has not been emptied before the loop, > like I asked in [a](14). > > The reason I had asked for emptying "mkimage" was two-fold: (i) I > considered that maybe you'd add "set -u" at the top of the script as > well, in which case the above "if" could fail ungracefully with > "unbound variable", (ii) "mkimage" is not a totally uncommon variable > name, and it could be inherited from the calling shell environment -- > in case the loop never matched. > > Now, I may have been wrong in that reasoning. But please, if you > think I'm wrong, say so. I think it's quite possible to convince me. > If you simply ignore a request from me, that's not a good use of my > time. I actually just missed this, sorry. I've updated the script. > Whenever I review the next version of a series, I go over every > single request I made under the last one, and verify whether it has > been addressed, or explicitly refuted in the discussion. If neither > happens, then I just don't know what to do, as I obviously have to > make the exact same request in the next round of review. It's > disappointing, and discourages me from continuing the review. Yes, sorry, it's probably because I didn't go through it point by point like I did this time. James > > + > > +# GRUB's rescue parser doesn't understand 'if'. > > +echo 'normal (memdisk)/grub.cfg' > "${basedir}/grub-bootstrap.cfg" > > + > > +# Now build a memdisk with the correct grub.cfg > > +rm -f -- "${basedir}/disk.fat" > > +mkfs.msdos -C -- "${basedir}/disk.fat" 64 > > +mcopy -i "${basedir}/disk.fat" -- "${basedir}/grub.cfg" ::grub.cfg > > + > > + > > +${mkimage} -O x86_64-efi \ > > + -p '(crypto0)' \ > > + -c "${basedir}/grub-bootstrap.cfg" \ > > + -m "${basedir}/disk.fat" \ > > + -o "${basedir}/grub.efi" \ > > + ${GRUB_MODULES} > > + > > +remove_efi=0 > > +echo "grub.efi generated in ${basedir}" > > > > Thanks > Laszlo >