Re: [PATCH] EFI: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson <hudson@xxxxxxxx>]

On Friday, September 4, 2020 9:02 AM, Roger Pau Monné <roger.pau@xxxxxxxxxx> 
wrote:
> On Fri, Aug 28, 2020 at 11:51:35AM +0000, Trammell Hudson wrote:
> >     diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> >     index 0273f79152..ba691b1890 100644
> >     --- a/xen/arch/x86/xen.lds.S
> >     +++ b/xen/arch/x86/xen.lds.S
> >     @@ -156,6 +156,7 @@ SECTIONS
> >     __note_gnu_build_id_end = .;
> >     } :note :text
> >     #elif defined(BUILD_ID_EFI)
> > -   . = ALIGN(32); /* workaround binutils section overlap bug */
>
> Is this a separate bugfix?
>
> That's the only change to the linker script, so is this bug somehow
> triggered by the new code additions this commit makes?
>
> It might also be nice to have some reference to the bug if possible,
> so that we know when the bug has been fixed and thus we can drop the
> workaround.

I've split this into a separate commit with links to the mailing list
discussion and bug fix applied to the binutils:
https://gitlab.com/xen-project/xen/-/merge_requests/4/diffs?commit_id=acfd8f85de8954bb08b726419a680e7ba5aba499

As Jan pointed out, it doesn't directly affect the xen build process
since the xen.efi is the end-product, although it does prevent users
from making further changes to the executable (such as merging the
pieces into the unified image) without having a bleeding edge version
of binutils.

> > -          file->need_to_free = true;
>
> Don't you need to set need_to_free after AllocatePages has returned
> successfully?

Yes, I think so. I've fixed this in the gitlab PR.

> Also the error handling in read_file is horrible to follow IMO.

Yeah, I'm a fan of early-return error handling rather than the "what ?: wtf"
style, although I left it as is.

> [...]
> For future patches it might be helpful to split non-functional changes
> into a separate patch. For example the inclusion of display_file_info
> could be a pre-patch, and that would help reduce the size of the diff.

Things like display_file_info() were separate commits that ended squashed
during one of the merges as the patch was being reworked.  They could
probably be out if someone wanted to rebase it again.

> > -   return secboot == 1 && setupmode == 0;
>
> Does this need to be strictly 1, or any value != 0?

We discussed this briefly here on xen-devel without any real conclusion;
the UEFI spec says that all other values are reserved. I'm not sure in practice
if any others ever show up.

> [...]
> I have to admit I know very little, but don't you need to verify the
> ramdisk also, like you verify the kernel? Or is the kernel the one
> that's supposed to verify it's ramdisk before using it?

With the unified image there is no need to do so; the xen.efi, config,
kernel, initrd, flash, and ucode are all signed as one file and the
shim protocol is not necessary.

For the non-unified case, well, that's what started me on this patch.
I was quite surprised that all of the Secure Boot support in Linux
distrbutions and Xen did not sign the initrd or command lines,
only the kernel image.  So, yes, it seems like it should be signed,
but that's not what the wider community decided to do.

> [...]
> > -   -   Derived from 
> > https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c
>
> Is it worth adding the commit that you used as the base for this file?

Done.  The code hasn't seen any updates in months, although it is worth noting
where it came from.

> > +/*
> > -   -   The "Unified" kernel image can be generated by adding additional
> > -   -   sections to the Xen EFI executable with objcopy, similar to how
> > -   -   systemd-boot uses the stub to add them to the Linux kernel:
> [...]
>
> This must be in a doc somewhere, likely in misc/efi.pandoc? People
> trying to use such functionality shouldn't resort to reading a comment
> on a source file IMO.

Thanks for the location suggestion. I wasn't sure where to store it.
Moved the comment into there and markdown'ed it.

I think I've addressed all of the style guide issues that you and others
brought up in the latest version on the gitlab site:

https://gitlab.com/xen-project/xen/-/merge_requests/4

--
Trammell