[PATCH 7/8] x86: enable 64-bit kernel boot from 64-bit U-Boot

[Park, Aiden <aiden.park@intel.com>]

Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Sunday, April 26, 2020 1:16 PM
> To: Park, Aiden <aiden.park@intel.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>; U-Boot Mailing List <u-
> boot at lists.denx.de>
> Subject: Re: [PATCH 7/8] x86: enable 64-bit kernel boot from 64-bit U-Boot
> 
> Hi Aiden,
> 
> On Tue, 21 Apr 2020 at 18:45, <aiden.park@intel.com> wrote:
> >
> > From: Aiden Park <aiden.park@intel.com>
> >
> > Signed-off-by: Aiden Park <aiden.park@intel.com>
> > ---
> >  arch/x86/cpu/x86_64/Makefile     |  2 +-
> >  arch/x86/cpu/x86_64/call64.S     | 21 +++++++++++++++++++++
> >  arch/x86/cpu/x86_64/cpu.c        | 10 ++++++++++
> >  arch/x86/include/asm/bootparam.h | 10 +++++++++-
> >  arch/x86/include/asm/zimage.h    |  2 +-
> >  arch/x86/lib/bootm.c             | 10 +++++++---
> >  arch/x86/lib/zimage.c            | 24 ++++++++++++++++--------
> >  7 files changed, 65 insertions(+), 14 deletions(-)  create mode
> > 100644 arch/x86/cpu/x86_64/call64.S
> >
> > diff --git a/arch/x86/cpu/x86_64/Makefile
> > b/arch/x86/cpu/x86_64/Makefile index 400f0ffe39..951e503a1f 100644
> > --- a/arch/x86/cpu/x86_64/Makefile
> > +++ b/arch/x86/cpu/x86_64/Makefile
> > @@ -3,4 +3,4 @@
> >  # Written by Simon Glass <sjg@chromium.org>  #
> >
> > -obj-y += cpu.o interrupts.o setjmp.o
> > +obj-y += cpu.o interrupts.o setjmp.o call64.o
> > diff --git a/arch/x86/cpu/x86_64/call64.S
> > b/arch/x86/cpu/x86_64/call64.S new file mode 100644 index
> > 0000000000..e2cfe15080
> > --- /dev/null
> > +++ b/arch/x86/cpu/x86_64/call64.S
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2020 Intel Corporation <www.intel.com>  */
> > +
> > +.code64
> > +.globl cpu_call64
> > +cpu_call64:
> > +       /*
> > +        * cpu_call64(ulong pgtable, ulong setup_base, ulong target)
> > +        *
> > +        * rdi - pgtable (unused - already in 64-bit with paging)
> > +        * rsi - setup_base
> > +        * rdx - target
> > +        *
> > +        */
> > +       cli
> > +       mov     %rdx, %rcx
> > +       mov     %rsi, %rdx
> > +       call    *%rcx
> > +       ret
> > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > index 90925e46ea..0730c43f1c 100644
> > --- a/arch/x86/cpu/x86_64/cpu.c
> > +++ b/arch/x86/cpu/x86_64/cpu.c
> > @@ -7,6 +7,7 @@
> >  #include <common.h>
> >  #include <cpu_func.h>
> >  #include <debug_uart.h>
> > +#include <asm/cpu.h>
> >
> >  /*
> >   * Global declaration of gd.
> > @@ -67,3 +68,12 @@ int x86_cpu_reinit_f(void)  {
> >         return 0;
> >  }
> > +
> > +int cpu_jump_to_64bit(ulong setup_base, ulong load_address) {
> > +       ulong target = load_address + 0x200;
> > +
> > +       cpu_call64(0, setup_base, target);
> > +
> > +       return -EFAULT;
> > +}
> > diff --git a/arch/x86/include/asm/bootparam.h
> > b/arch/x86/include/asm/bootparam.h
> > index d961dddc9e..73c94a33ec 100644
> > --- a/arch/x86/include/asm/bootparam.h
> > +++ b/arch/x86/include/asm/bootparam.h
> > @@ -59,7 +59,15 @@ struct setup_header {
> >         __u32   initrd_addr_max;
> >         __u32   kernel_alignment;
> >         __u8    relocatable_kernel;
> > -       __u8    _pad2[3];
> > +       __u8    min_alignment;
> > +       __u16   xloadflags;
> > +#define XLF_KERNEL_64                  BIT(0)
> > +#define XLF_CAN_BE_LOADED_ABOVE_4G     BIT(1)
> > +#define XLF_EFI_HANDOVER_32            BIT(2)
> > +#define XLF_EFI_HANDOVER_64            BIT(3)
> > +#define XLF_EFI_KEXEC                  BIT(4)
> > +#define XLF_5LEVEL                     BIT(5)
> > +#define XLF_5LEVEL_ENABLED             BIT(6)
> >         __u32   cmdline_size;
> >         __u32   hardware_subarch;
> >         __u64   hardware_subarch_data;
> > diff --git a/arch/x86/include/asm/zimage.h
> > b/arch/x86/include/asm/zimage.h index 80e128ccf3..cadeb04168 100644
> > --- a/arch/x86/include/asm/zimage.h
> > +++ b/arch/x86/include/asm/zimage.h
> > @@ -31,7 +31,7 @@
> >  #define ZIMAGE_LOAD_ADDR   0x10000
> >
> >  struct boot_params *load_zimage(char *image, unsigned long kernel_size,
> > -                               ulong *load_addressp);
> > +                               ulong *load_addressp, bool
> > + *image_64bit);
> 
> Please put a 'p' on the end, so image_64bitp, since it a return value.
Thanks. I will do that.

> 
> >  int setup_zimage(struct boot_params *setup_base, char *cmd_line, int
> auto_boot,
> >                  unsigned long initrd_addr, unsigned long
> > initrd_size);  void setup_video(struct screen_info *screen_info); diff
> > --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index
> > 07d8f1f279..ebed9e4605 100644
> > --- a/arch/x86/lib/bootm.c
> > +++ b/arch/x86/lib/bootm.c
> > @@ -74,6 +74,7 @@ static int boot_prep_linux(bootm_headers_t *images)
> >         void *data = NULL;
> >         size_t len;
> >         int ret;
> > +       bool image_64bit;
> >
> >  #ifdef CONFIG_OF_LIBFDT
> >         if (images->ft_len) {
> > @@ -116,7 +117,8 @@ static int boot_prep_linux(bootm_headers_t *images)
> >                 ulong load_address;
> >                 char *base_ptr;
> >
> > -               base_ptr = (char *)load_zimage(data, len, &load_address);
> > +               base_ptr = (char *)load_zimage(data, len, &load_address,
> > +                                              &image_64bit);
> >                 if (!base_ptr) {
> >                         puts("## Kernel loading failed ...\n");
> >                         goto error;
> > @@ -124,6 +126,10 @@ static int boot_prep_linux(bootm_headers_t
> *images)
> >                 images->os.load = load_address;
> >                 cmd_line_dest = base_ptr + COMMAND_LINE_OFFSET;
> >                 images->ep = (ulong)base_ptr;
> > +#if CONFIG_IS_ENABLED(X86_64)
> > +               if (image_64bit)
> 
> if (CONFIG_IS_ENABLED(X86_64) && image_64bit)
> 
> > +                       images->os.arch = IH_ARCH_X86_64; #endif
> >         } else if (images->ep) {
> >                 cmd_line_dest = (void *)images->ep + COMMAND_LINE_OFFSET;
> >         } else {
> > @@ -164,9 +170,7 @@ int boot_linux_kernel(ulong setup_base, ulong
> load_address, bool image_64bit)
> >                  * TODO(sjg at chromium.org): Support booting both 32-bit and
> >                  * 64-bit kernels from 64-bit U-Boot.
> >                  */
> > -#if !CONFIG_IS_ENABLED(X86_64)
> 
> if()
I will do that.

> 
> >                 return cpu_jump_to_64bit(setup_base, load_address);
> > -#endif
> >         } else {
> >                 /*
> >                 * Set %ebx, %ebp, and %edi to 0, %esi to point to the
> > diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c index
> > ffc09630b7..c84e2d39b2 100644
> > --- a/arch/x86/lib/zimage.c
> > +++ b/arch/x86/lib/zimage.c
> > @@ -129,7 +129,7 @@ static int setup_device_tree(struct setup_header
> > *hdr, const void *fdt_blob)  }
> >
> >  struct boot_params *load_zimage(char *image, unsigned long kernel_size,
> > -                               ulong *load_addressp)
> > +                               ulong *load_addressp, bool
> > + *image_64bit)
> 
> image_64bitp
I will do that.

> 
> >  {
> >         struct boot_params *setup_base;
> >         int setup_size;
> > @@ -179,6 +179,9 @@ struct boot_params *load_zimage(char *image,
> unsigned long kernel_size,
> >         big_image = (bootproto >= 0x0200) &&
> >                     (hdr->loadflags & BIG_KERNEL_FLAG);
> >
> > +       /* Determine 64-bit kernel */
> > +       *image_64bit = (hdr->xloadflags & XLF_KERNEL_64) ? true :
> > + false;
> > +
> 
> Drop ()
> 
> In fact I suspect you can do:
> 
> *image_64bit = hdr->xloadflags & XLF_KERNEL_64)
I will do that.

> 
> >         /* Determine load address */
> >         if (big_image)
> >                 *load_addressp = BZIMAGE_LOAD_ADDR; @@ -313,12 +316,13
> > @@ void __setup_pcat_compatibility(void)  int do_zboot(cmd_tbl_t
> > *cmdtp, int flag, int argc, char *const argv[])  {
> >         struct boot_params *base_ptr;
> > -       void *bzImage_addr = NULL;
> > +       void *bzimage_addr = NULL;
> 
> Good change, but really should be in a separate patch.
Let me make a separate patch for this.

> 
> >         ulong load_address;
> >         char *s;
> > -       ulong bzImage_size = 0;
> > +       ulong bzimage_size = 0;
> >         ulong initrd_addr = 0;
> >         ulong initrd_size = 0;
> > +       bool image_64bit;
> >
> >         disable_interrupts();
> >
> > @@ -333,11 +337,11 @@ int do_zboot(cmd_tbl_t *cmdtp, int flag, int argc,
> char *const argv[])
> >         }
> >
> >         if (s)
> > -               bzImage_addr = (void *)simple_strtoul(s, NULL, 16);
> > +               bzimage_addr = (void *)simple_strtoul(s, NULL, 16);
> >
> >         if (argc >= 3) {
> >                 /* argv[2] holds the size of the bzImage */
> > -               bzImage_size = simple_strtoul(argv[2], NULL, 16);
> > +               bzimage_size = simple_strtoul(argv[2], NULL, 16);
> >         }
> >
> >         if (argc >= 4)
> > @@ -346,8 +350,13 @@ int do_zboot(cmd_tbl_t *cmdtp, int flag, int argc,
> char *const argv[])
> >                 initrd_size = simple_strtoul(argv[4], NULL, 16);
> >
> >         /* Lets look for */
> > -       base_ptr = load_zimage(bzImage_addr, bzImage_size, &load_address);
> > +       base_ptr = load_zimage(bzimage_addr, bzimage_size, &load_address,
> > +                              &image_64bit); #if
> > +!CONFIG_IS_ENABLED(X86_64)
> 
> if()
> 
> But why not just set it always?
This was just to keep original boot flow in zboot to boot 32-bit kernel entry from 32-bit U-Boot.
I also agree with you. 32-bit U-Boot is able to boot 64-bit kernel entry with 64-bit paging.
Let me remove this.

> 
> > +       image_64bit = false;
> > +#endif
> >
> > +       /* we assume that the kernel is in place */
> >         if (!base_ptr) {
> >                 puts("## Kernel loading failed ...\n");
> >                 return -1;
> > @@ -358,8 +367,7 @@ int do_zboot(cmd_tbl_t *cmdtp, int flag, int argc,
> char *const argv[])
> >                 return -1;
> >         }
> >
> > -       /* we assume that the kernel is in place */
> > -       return boot_linux_kernel((ulong)base_ptr, load_address, false);
> > +       return boot_linux_kernel((ulong)base_ptr, load_address,
> > + image_64bit);
> >  }
> >
> >  U_BOOT_CMD(
> > --
> > 2.20.1
> >
> 
> Regards,
> Simon

Best Regards,
Aiden