Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson <hudson@xxxxxxxx>]

On Monday, September 14, 2020 6:06 AM, Roger Pau Monné <roger.pau@xxxxxxxxxx> 
wrote:
> On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote:
> > [...]
> > It is inspired by systemd-boot's unified kernel technique and borrows the
> > function to locate PE sections from systemd's LGPL'ed code. During EFI
> > boot, Xen looks at its own loaded image to locate the PE sections for
> > the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd
> > (`.initrd`), and XSM config (`.xsm`), which are included after building
>
> Could we name this kernel_payload or maybe just payload instead of
> initrd?
>
> That's a Linux specific concept.

Perhaps "ramdisk" is better, since that is the name of the module in the
Xen config file?  That was what I used elsewhere (and messed up in the docs,
as you had noticed).

> [...]
> > -   --change-section-vma .initrd=0xffff82d042000000 \
>
> Why do you need to adjust the VMA, is this not set to a suitable
> default value?
>
> How can users find a suitable VMA value?

The default is to put the new sections at virtual address 0, which causes
load errors.  It seemed to be necessary to have it above the hypervisor
image, although I'm also borrowing that from the systemd-boot code.
I wish objcopy had an "--append-section" that would add after the last
section in the file...

An earlier version of the patch series had a shell script to do this process,
although it was not as general as people wanted, so it was dropped in favor of
documenting how to build the images with objcopy.

> [...]
> > -   file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
>
> This already returns a void *, so there's no need for the cast.

It returns const void *, but file has a non-const pointer.  Perhaps files should
be immutable and that could be addressed in a const-correctness patch series.

The style guide issues will also be addressed in the v4 of the patch, to
be posted soon.

--
Trammell