Hi Pinfang,
On Mon, 17 Apr 2023 10:55:13 +0800
Pingfan Liu <piliu(a)redhat.com> wrote:
On Fri, Apr 14, 2023 at 10:50 PM Philipp Rudo
<prudo(a)redhat.com> wrote:
>
> On Fri, 14 Apr 2023 10:55:22 +0800
> Pingfan Liu <piliu(a)redhat.com> wrote:
>
> > Hi Philipp,
> >
> > It is an interesting topic. And please see my inline comments.
> >
> > On Tue, Apr 11, 2023 at 11:27 PM Philipp Rudo <prudo(a)redhat.com> wrote:
> > >
> > > A Unified Kernel Image (UKI) is a single EFI PE executable combining an
> > > EFI stub, a kernel image, an initrd image, and the kernel command line.
> > > They are defined in the Boot Loader Specification [1] as type #2
> > > entries. UKIs have the advantage that all code as well as meta data that
> > > is required to boot the system, not only the kernel image, is combined
> > > in a single PE file and can be signed for EFI SecureBoot. This extends
> > > the coverage of SecureBoot extensively.
> > >
> > > For RHEL support for UKI were included into kernel-ark with 16c7e3ee836e
> > > ("redhat: Add sub-RPM with a EFI unified kernel image for virtual
> > > machines").
> > >
> > > There are two problems with UKIs from the kdump point of view at the
> > > moment. First, they cannot be directly loaded via kexec_file_load and
> > > second, the initrd included isn't suitable for kdump. In order to
enable
> > > kdump on systems with UKIs build the kdump initrd as usual and extract
> > > the kernel image before loading the crash kernel.
> > >
> > > [1]
https://uapi-group.org/specifications/specs/boot_loader_specification/
> > >
> > > Signed-off-by: Philipp Rudo <prudo(a)redhat.com>
> > > ---
> > > kdump-lib.sh | 21 ++++++++++++++++++++-
> > > kdumpctl | 12 +++++++++++-
> > > 2 files changed, 31 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kdump-lib.sh b/kdump-lib.sh
> > > index 44ea9a4..ddbb8ed 100755
> > > --- a/kdump-lib.sh
> > > +++ b/kdump-lib.sh
> > > @@ -11,6 +11,11 @@ fi
> > > FADUMP_ENABLED_SYS_NODE="/sys/kernel/fadump_enabled"
> > > FADUMP_REGISTER_SYS_NODE="/sys/kernel/fadump_registered"
> > >
> > > +is_uki()
> > > +{
> > > + [[ -f /boot/efi/EFI/Linux/vmlinuz-$1-virt.efi ]]
> >
> > Could it be achieved by checking the sections' name in the binary? It
> > will be more robust since the naming of the section is part of UKI
> > specification.
>
> The BLS contains a naming convention for the UKIs. I thought that
Referring to BLS, it claims "the files must have the extension .efi."
I think this is a necessary but not sufficient condition. And on
aarch64, a zboot image can also be PE format. Am I missing anything?
I don't fully get your question. For me the PE format is simply the
container that is required by the EFI standard to be used for images
(at least when you want to support SecureBoot). But that has nothing to
do with the .efi suffix or other requirements of the BLS. In particular
the zboot image should never have a .efi suffix but should only be
referenced by a type 1 entry (aka. the linux entry in the .conf file)
if you follow the BLS.
Anyway...
> relying on the convention would be sufficient. Especially as
it's much
> more light weight compared to calling objdump and parsing for the
> 'right' sections same. But I don't have a strong opinion on it. So we
> can also add a check for the section names.
>
If it turns out to be a necessary and sufficient condition, I am happy
to withdraw my suggestion.
... I had some second thoughts and I think I will take over your
suggestion. That is because of the other feature I'm working on right
now where you can specify the image to be used by a --kernel option. If
that is used we cannot rely on file naming but have to look at the
file content instead.
Thanks
Philipp
> BTW, just noticed that I forgot to add the binutils as
dependency in
> the .spec file...
>
> > > +}
> > > +
> > > is_fadump_capable()
> > > {
> > > # Check if firmware-assisted dump is enabled
> > > @@ -631,6 +636,11 @@ prepare_kdump_kernel()
> > > local dir img boot_dirlist boot_imglist kdump_kernel machine_id
> > > read -r machine_id < /etc/machine-id
> > >
> > > + if is_uki $kdump_kernelver; then
> > > + echo
"/boot/efi/EFI/Linux/vmlinuz-$kdump_kernelver-virt.efi"
> > > + return
> > > + fi
> > > +
> > > boot_dirlist=${KDUMP_BOOTDIR:-"/boot /boot/efi /efi /"}
> > > boot_imglist="$KDUMP_IMG-$kdump_kernelver$KDUMP_IMG_EXT
$machine_id/$kdump_kernelver/$KDUMP_IMG"
> > >
> > > @@ -705,7 +715,11 @@ prepare_kdump_bootinfo()
> > > fi
> > >
> > > # Set KDUMP_BOOTDIR to where kernel image is stored
> > > - KDUMP_BOOTDIR=$(dirname "$KDUMP_KERNEL")
> > > + if is_uki $KDUMP_KERNELVER; then
> > > + KDUMP_BOOTDIR=/boot
> > > + else
> > > + KDUMP_BOOTDIR=$(dirname "$KDUMP_KERNEL")
> > > + fi
> > >
> > > # Default initrd should just stay aside of kernel image, try to
find it in KDUMP_BOOTDIR
> > > boot_initrdlist="initramfs-$KDUMP_KERNELVER.img initrd"
> > > @@ -858,6 +872,11 @@ prepare_cmdline()
> > > # may cause the hot-remove of some pci hotplug device.
> > > is_aws_aarch64 && out=$(echo "$out" | sed -e
"/\<irqpoll\>//")
> > >
> > > + # Always disable gpt-auto-generator as it hangs during boot of
the
> > > + # crash kernel. Furthermore we know which disk will be used for
dumping
> > > + # (if at all) and add it explicitly.
> > > + is_uki $KDUMP_KERNELVER &&
out+="rd.systemd.gpt_auto=no "
> > > +
> > > # Trim unnecessary whitespaces
> > > echo "$out" | sed -e "s/^ *//g" -e "s/
*$//g" -e "s/ \+/ /g"
> > > }
> > > diff --git a/kdumpctl b/kdumpctl
> > > index 9d02275..61b2b08 100755
> > > --- a/kdumpctl
> > > +++ b/kdumpctl
> > > @@ -643,7 +643,7 @@ function remove_kdump_kernel_key()
> > > # as the currently running kernel.
> > > load_kdump()
> > > {
> > > - local ret
> > > + local ret uki
> > >
> > > KEXEC_ARGS=$(prepare_kexec_args "${KEXEC_ARGS}")
> > > KDUMP_COMMANDLINE=$(prepare_cmdline
"${KDUMP_COMMANDLINE}" "${KDUMP_COMMANDLINE_REMOVE}"
"${KDUMP_COMMANDLINE_APPEND}")
> > > @@ -656,6 +656,16 @@ load_kdump()
> > > load_kdump_kernel_key
> > > fi
> > >
> > > + if is_uki $KDUMP_KERNELVER; then
> > > + uki=$KDUMP_KERNEL
> > > + KDUMP_KERNEL=$KDUMP_TMPDIR/vmlinuz
> > > + objcopy -O binary --only-section .linux "$uki"
"$KDUMP_KERNEL"
> >
> > Out of topic, is the .linux signed? If not, we need to demand it in
> > the kernel.spec.
>
> AFAIK the .linux isn't required to be signed by the standard because
> the whole UKI can be/is signed. For RHEL however the .linux is signed
> as the UKI is created after a normal kernel build with the signed
> bzImage. And that's good. Because otherwise we couldn't load it via
> kexec_file_load on a locked down kernel.
>
Yeah, this is my intention.
Thanks,
Pingfan