[edk2-devel] [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package

[James Bottomley]

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.<BR>
> > +#  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.<BR>
> > +  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.<BR>
> > +  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.<BR>
> > +  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
> 




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67871): https://edk2.groups.io/g/devel/message/67871
Mute This Topic: https://groups.io/mt/78455898/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-