Re: [PATCHv5 0/8] arm64: zboot support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dave,

Thanks for your insight. Please see the comments inline below.

On Wed, Jul 19, 2023 at 11:00 AM Dave Young <dyoung@xxxxxxxxxx> wrote:
>
> Hi Pingfan, Simon,
>
> On 07/17/23 at 09:07pm, Pingfan Liu wrote:
> > As more complicated capsule kernel format occurs like zboot, where the
> > compressed kernel is stored as a payload. The straight forward
> > decompression can not meet the demand.
> >
> > As the first step, on aarch64, reading in the kernel file in a probe
> > method and decide how to unfold the content by the method itself.
> >
> > This series introduce a new image probe interface probe2(), which
> > returns three factors: kernel buffer, kernel size and kernel fd through
> > a struct parsed_info.
> > -1. the parsed kernel_buf should be returned so that it can be used by
> > the image load method later.
> > -2. the final fd passed to sys_kexec_file_load, since aarch64 kernel can
> > only work with Image format, the outer payload should be stripped and a
> > temporary file of Image should be created.
>
> I took a look at the Image.gz file load code, the current code can be
> simplified with passing a fd directly instead of creating temp files via
> memfd_create with the already decompressed kernel_buf.
>
> The current file load is like below:
>
> do_kexec_file_load():
>   1.slurp_decompress_file
>     2. probe
>       3. load
>         4. kexec_file_load
>
> In step 1, the Image.gz has been decompressed to kernel_buf, so just
> create a virtual memfd copy to it, then save the virtual fd for step 4
> use.
>
> Otherwise in step 2 it is some sanity checking, step 3 is setting
> something else eg. initrd_fd, cmdline. With the changes below Image and
> Image.gz will share same code. I think you can add the zboot
> detection/checking code in the Image probe, load functions, with a new
> info->kernel_fd, you can decompress the zboot kernel_buf and save to
> another virtual memfd, and set to the info->kernel_fd.  Then in step 4
> the kexec_file_load can just use it.
>

This only results in a minor change in the interface, not like mine. I
prefer this method.

> The kernel_buf itself is only used for sanity checking of the formats,
> kernel only needs a file fd, so I think it should be fine and easier
> than the original ways.
>

Overall, this new method minimally affects the function interface, in
addition to the code simplification.
I will try to take it in the next version.

> Thoughts?
>
> ---
>  kexec/arch/arm64/Makefile             |    3
>  kexec/arch/arm64/kexec-arm64.c        |    1
>  kexec/arch/arm64/kexec-arm64.h        |    6
>  kexec/arch/arm64/kexec-image-arm64.c  |    2
>  kexec/arch/arm64/kexec-zImage-arm64.c |  226 ----------------------------------
>  kexec/kexec.c                         |   55 +++++---
>  kexec/kexec.h                         |    1
>  7 files changed, 39 insertions(+), 255 deletions(-)
>
> Index: kexec-tools/kexec/arch/arm64/Makefile
> ===================================================================
> --- kexec-tools.orig/kexec/arch/arm64/Makefile
> +++ kexec-tools/kexec/arch/arm64/Makefile
> @@ -15,8 +15,7 @@ arm64_KEXEC_SRCS += \
>         kexec/arch/arm64/kexec-arm64.c \
>         kexec/arch/arm64/kexec-elf-arm64.c \
>         kexec/arch/arm64/kexec-uImage-arm64.c \
> -       kexec/arch/arm64/kexec-image-arm64.c \
> -       kexec/arch/arm64/kexec-zImage-arm64.c
> +       kexec/arch/arm64/kexec-image-arm64.c
>
>  arm64_UIMAGE = kexec/kexec-uImage.c
>
> Index: kexec-tools/kexec/arch/arm64/kexec-arm64.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/arm64/kexec-arm64.c
> +++ kexec-tools/kexec/arch/arm64/kexec-arm64.c
> @@ -74,7 +74,6 @@ struct file_type file_type[] = {
>         {"vmlinux", elf_arm64_probe, elf_arm64_load, elf_arm64_usage},
>         {"Image", image_arm64_probe, image_arm64_load, image_arm64_usage},
>         {"uImage", uImage_arm64_probe, uImage_arm64_load, uImage_arm64_usage},
> -       {"zImage", zImage_arm64_probe, zImage_arm64_load, zImage_arm64_usage},
>  };
>
>  int file_types = sizeof(file_type) / sizeof(file_type[0]);
> Index: kexec-tools/kexec/arch/arm64/kexec-arm64.h
> ===================================================================
> --- kexec-tools.orig/kexec/arch/arm64/kexec-arm64.h
> +++ kexec-tools/kexec/arch/arm64/kexec-arm64.h
> @@ -44,12 +44,6 @@ int uImage_arm64_load(int argc, char **a
>                       struct kexec_info *info);
>  void uImage_arm64_usage(void);
>
> -int zImage_arm64_probe(const char *kernel_buf, off_t kernel_size);
> -int zImage_arm64_load(int argc, char **argv, const char *kernel_buf,
> -       off_t kernel_size, struct kexec_info *info);
> -void zImage_arm64_usage(void);
> -
> -
>  extern off_t initrd_base;
>  extern off_t initrd_size;
>
> Index: kexec-tools/kexec/arch/arm64/kexec-image-arm64.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/arm64/kexec-image-arm64.c
> +++ kexec-tools/kexec/arch/arm64/kexec-image-arm64.c
> @@ -114,6 +114,6 @@ exit:
>  void image_arm64_usage(void)
>  {
>         printf(
> -"     An ARM64 binary image, uncompressed, big or little endian.\n"
> +"     An ARM64 binary image, compressed or not, big or little endian.\n"
>  "     Typically an Image file.\n\n");
>  }
> Index: kexec-tools/kexec/arch/arm64/kexec-zImage-arm64.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/arm64/kexec-zImage-arm64.c
> +++ /dev/null
> @@ -1,226 +0,0 @@
> -/*
> - * ARM64 kexec zImage (Image.gz) support.
> - *
> - * Several distros use 'make zinstall' rule inside
> - * 'arch/arm64/boot/Makefile' to install the arm64
> - * Image.gz compressed file inside the boot destination
> - * directory (for e.g. /boot).
> - *
> - * Currently we cannot use kexec_file_load() to load vmlinuz
> - * (or Image.gz).
> - *
> - * To support Image.gz, we should:
> - * a). Copy the contents of Image.gz to a temporary file.
> - * b). Decompress (gunzip-decompress) the contents inside the
> - *     temporary file.
> - * c). Pass the 'fd' of the temporary file to the kernel space.
> - *
> - * So basically the kernel space still gets a decompressed
> - * kernel image to load via kexec-tools.
> - */
> -
> -#define _GNU_SOURCE
> -
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <limits.h>
> -#include <stdlib.h>
> -#include "crashdump-arm64.h"
> -#include "image-header.h"
> -#include "kexec.h"
> -#include "kexec-arm64.h"
> -#include "kexec-syscall.h"
> -#include "kexec-zlib.h"
> -#include "arch/options.h"
> -
> -#define FILENAME_IMAGE         "/tmp/ImageXXXXXX"
> -
> -/* Returns:
> - * -1 : in case of error/invalid format (not a valid Image.gz format.
> - * fd : File descriptor of the temp file containing the decompressed
> - *      Image.
> - */
> -int zImage_arm64_probe(const char *kernel_buf, off_t kernel_size)
> -{
> -       int ret = -1;
> -       int fd = 0;
> -       int kernel_fd = 0;
> -       char *fname = NULL;
> -       char *kernel_uncompressed_buf = NULL;
> -       const struct arm64_image_header *h;
> -
> -       if (!is_zlib_file(kernel_buf, &kernel_size)) {
> -               dbgprintf("%s: Not an zImage file (Image.gz).\n", __func__);
> -               return -1;
> -       }
> -
> -       if (!(fname = strdup(FILENAME_IMAGE))) {
> -               dbgprintf("%s: Can't duplicate strings %s\n", __func__,
> -                               fname);
> -               return -1;
> -       }
> -
> -       if ((fd = mkstemp(fname)) < 0) {
> -               dbgprintf("%s: Can't open file %s\n", __func__,
> -                               fname);
> -               ret = -1;
> -               goto fail_mkstemp;
> -       }
> -
> -       kernel_uncompressed_buf =
> -               (char *) calloc(kernel_size, sizeof(off_t));
> -       if (!kernel_uncompressed_buf) {
> -               dbgprintf("%s: Can't calloc %ld bytes\n",
> -                               __func__, kernel_size);
> -               ret= -ENOMEM;
> -               goto fail_calloc;
> -       }
> -
> -       /* slurp in the input kernel */
> -       dbgprintf("%s: ", __func__);
> -       kernel_uncompressed_buf = slurp_decompress_file(kernel_buf,
> -                                                       &kernel_size);
> -
> -       /* check for correct header magic */
> -       if (kernel_size < sizeof(struct arm64_image_header)) {
> -               dbgprintf("%s: No arm64 image header.\n", __func__);
> -               ret = -1;
> -               goto fail_bad_header;
> -       }
> -
> -       h = (const struct arm64_image_header *)(kernel_uncompressed_buf);
> -
> -       if (!arm64_header_check_magic(h)) {
> -               dbgprintf("%s: Bad arm64 image header.\n", __func__);
> -               ret = -1;
> -               goto fail_bad_header;
> -       }
> -
> -       if (write(fd, kernel_uncompressed_buf,
> -                               kernel_size) != kernel_size) {
> -               dbgprintf("%s: Can't write the uncompressed file %s\n",
> -                               __func__, fname);
> -               ret = -1;
> -               goto fail_bad_header;
> -       }
> -
> -       close(fd);
> -
> -       /* Open the tmp file again, this time in O_RDONLY mode, as
> -        * opening the file in O_RDWR and calling kexec_file_load()
> -        * causes the kernel to return -ETXTBSY
> -        */
> -       kernel_fd = open(fname, O_RDONLY);
> -       if (kernel_fd == -1) {
> -               dbgprintf("%s: Failed to open file %s\n",
> -                               __func__, fname);
> -               ret = -1;
> -               goto fail_bad_header;
> -       }
> -
> -       unlink(fname);
> -
> -       free(kernel_uncompressed_buf);
> -       free(fname);
> -
> -       return kernel_fd;
> -
> -fail_bad_header:
> -       free(kernel_uncompressed_buf);
> -
> -fail_calloc:
> -       if (fd >= 0)
> -               close(fd);
> -
> -       unlink(fname);
> -
> -fail_mkstemp:
> -       free(fname);
> -
> -       return ret;
> -}
> -
> -int zImage_arm64_load(int argc, char **argv, const char *kernel_buf,
> -       off_t kernel_size, struct kexec_info *info)
> -{
> -       const struct arm64_image_header *header;
> -       unsigned long kernel_segment;
> -       int result;
> -
> -       if (info->file_mode) {
> -               if (arm64_opts.initrd) {
> -                       info->initrd_fd = open(arm64_opts.initrd, O_RDONLY);
> -                       if (info->initrd_fd == -1) {
> -                               fprintf(stderr,
> -                                       "Could not open initrd file %s:%s\n",
> -                                       arm64_opts.initrd, strerror(errno));
> -                               result = EFAILED;
> -                               goto exit;
> -                       }
> -               }
> -
> -               if (arm64_opts.command_line) {
> -                       info->command_line = (char *)arm64_opts.command_line;
> -                       info->command_line_len =
> -                                       strlen(arm64_opts.command_line) + 1;
> -               }
> -
> -               return 0;
> -       }
> -
> -       header = (const struct arm64_image_header *)(kernel_buf);
> -
> -       if (arm64_process_image_header(header))
> -               return EFAILED;
> -
> -       kernel_segment = arm64_locate_kernel_segment(info);
> -
> -       if (kernel_segment == ULONG_MAX) {
> -               dbgprintf("%s: Kernel segment is not allocated\n", __func__);
> -               result = EFAILED;
> -               goto exit;
> -       }
> -
> -       dbgprintf("%s: kernel_segment: %016lx\n", __func__, kernel_segment);
> -       dbgprintf("%s: text_offset:    %016lx\n", __func__,
> -               arm64_mem.text_offset);
> -       dbgprintf("%s: image_size:     %016lx\n", __func__,
> -               arm64_mem.image_size);
> -       dbgprintf("%s: phys_offset:    %016lx\n", __func__,
> -               arm64_mem.phys_offset);
> -       dbgprintf("%s: vp_offset:      %016lx\n", __func__,
> -               arm64_mem.vp_offset);
> -       dbgprintf("%s: PE format:      %s\n", __func__,
> -               (arm64_header_check_pe_sig(header) ? "yes" : "no"));
> -
> -       /* create and initialize elf core header segment */
> -       if (info->kexec_flags & KEXEC_ON_CRASH) {
> -               result = load_crashdump_segments(info);
> -               if (result) {
> -                       dbgprintf("%s: Creating eflcorehdr failed.\n",
> -                                                               __func__);
> -                       goto exit;
> -               }
> -       }
> -
> -       /* load the kernel */
> -       add_segment_phys_virt(info, kernel_buf, kernel_size,
> -                       kernel_segment + arm64_mem.text_offset,
> -                       arm64_mem.image_size, 0);
> -
> -       /* load additional data */
> -       result = arm64_load_other_segments(info, kernel_segment
> -               + arm64_mem.text_offset);
> -
> -exit:
> -       if (result)
> -               fprintf(stderr, "kexec: load failed.\n");
> -       return result;
> -}
> -
> -void zImage_arm64_usage(void)
> -{
> -       printf(
> -"     An ARM64 zImage, compressed, big or little endian.\n"
> -"     Typically an Image.gz or Image.lzma file.\n\n");
> -}
> Index: kexec-tools/kexec/kexec.c
> ===================================================================
> --- kexec-tools.orig/kexec/kexec.c
> +++ kexec-tools/kexec/kexec.c
> @@ -638,6 +638,21 @@ char *slurp_decompress_file(const char *
>         return kernel_buf;
>  }
>
> +int copybuf_memfd(const char *kernel_buf, size_t size)
> +{
> +       int fd, count;
> +
> +       fd = memfd_create("kernel", MFD_ALLOW_SEALING);
> +       if (fd == -1)
> +               return fd;
> +
> +       count = write(fd, kernel_buf, size);
> +       if (count < 0)
> +               return -1;
> +
> +       return fd;
> +}
> +
>  static void update_purgatory(struct kexec_info *info)
>  {
>         static const uint8_t null_buf[256];
> @@ -1263,7 +1278,7 @@ static int do_kexec_file_load(int filein
>                         unsigned long flags) {
>
>         char *kernel;
> -       int kernel_fd, i;
> +       int kernel_fd, i, fd;
>         struct kexec_info info;
>         int ret = 0;
>         char *kernel_buf;
> @@ -1277,6 +1292,7 @@ static int do_kexec_file_load(int filein
>         info.kexec_flags = flags;
>
>         info.file_mode = 1;
> +       info.kernel_fd = -1;
>         info.initrd_fd = -1;
>
>         if (!is_kexec_file_load_implemented())
> @@ -1299,22 +1315,16 @@ static int do_kexec_file_load(int filein
>
>         /* slurp in the input kernel */
>         kernel_buf = slurp_decompress_file(kernel, &kernel_size);
> +       fd = copybuf_memfd(kernel_buf, kernel_size);
> +       if (fd < 0)
> +               fprintf(stderr, "Failed to copy decompressed buf\n");
> +       else {
> +               kernel_fd = fd;
> +       }
>
>         for (i = 0; i < file_types; i++) {
> -#ifdef __aarch64__
> -               /* handle Image.gz like cases */
> -               if (is_zlib_file(kernel, &kernel_size)) {
> -                       if ((ret = file_type[i].probe(kernel, kernel_size)) >= 0) {

Just a clarification for this:
After removing the condition macro snippet, the later patches, which
implements pez_arm64_probe() should take the original kernel image
instead of kernel file as the first parameter. It is fine.


Thanks,

Pingfan
> -                               kernel_fd = ret;
> -                               break;
> -                       }
> -               } else
> -                       if (file_type[i].probe(kernel_buf, kernel_size) >= 0)
> -                               break;
> -#else
>                 if (file_type[i].probe(kernel_buf, kernel_size) >= 0)
>                         break;
> -#endif
>         }
>
>         if (i == file_types) {
> @@ -1324,12 +1334,19 @@ static int do_kexec_file_load(int filein
>                 return EFAILED;
>         }
>
> -       ret = file_type[i].load(argc, argv, kernel_buf, kernel_size, &info);
> -       if (ret < 0) {
> -               fprintf(stderr, "Cannot load %s\n", kernel);
> -               close(kernel_fd);
> -               return ret;
> -       }
> +       ret = file_type[i].load(argc, argv, kernel_buf, kernel_size, &info);
> +       if (ret < 0) {
> +               fprintf(stderr, "Cannot load %s\n", kernel);
> +               close(kernel_fd);
> +               return ret;
> +       }
> +
> +       /*
> +       * image type specific load functioin detect the capsule kernel type
> +       * and create another fd for file load. For example the zboot kernel.
> +       */
> +       if (info.kernel_fd != -1)
> +               kernel_fd = info.kernel_fd;
>
>         /*
>          * If there is no initramfs, set KEXEC_FILE_NO_INITRAMFS flag so that
> Index: kexec-tools/kexec/kexec.h
> ===================================================================
> --- kexec-tools.orig/kexec/kexec.h
> +++ kexec-tools/kexec/kexec.h
> @@ -164,6 +164,7 @@ struct kexec_info {
>         unsigned long file_mode :1;
>
>         /* Filled by kernel image processing code */
> +       int kernel_fd;
>         int initrd_fd;
>         char *command_line;
>         int command_line_len;
>


_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux