Re: [PATCH v2 10/16] LoongArch: Boot Image bits

[Yao Zi <me@ziyao.cc>]

On Thu, Jul 02, 2026 at 11:26:23AM +0100, Simon Glass wrote:
> Hi Yao,
> 
> On 2026-07-01T11:17:53, Yao Zi <me@ziyao.cc> wrote:
> > LoongArch: Boot Image bits
> >
> > Implement loading and booting functions for LoongArch
> > standard kernel image as per spec.
> >
> > LoongArch kernel do expect us to fake a efi systemtable
> > for passing fdt to kernel, we don't need to implement any
> > EFI functions for kernel because it won't look into anything
> > beside devicetree from that table if we tell kernel we are
> > not efi compatible by setting a0 boot argument to zero.
> >
> > Link: https://docs.kernel.org/arch/loongarch/booting.html
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > Signed-off-by: Yao Zi <me@ziyao.cc>
> >
> > arch/loongarch/lib/Makefile |   2 +
> >  arch/loongarch/lib/bootm.c  | 163 ++++++++++++++++++++++++++++++++++++++++++++
> >  arch/loongarch/lib/image.c  |  66 ++++++++++++++++++
> >  cmd/Kconfig                 |   2 +-
> >  4 files changed, 232 insertions(+), 1 deletion(-)

...

> > diff --git a/arch/loongarch/lib/bootm.c b/arch/loongarch/lib/bootm.c
> > @@ -0,0 +1,163 @@
> > +/* LoongArch do expect a EFI style systab */
> > +static int generate_systab(struct bootm_headers *images)
> > +{
> > +     const int nr_cfgtab = 1;
> > +     struct bd_info *kbd = images->kbd;
> > +     struct efi_system_table *systab;
> > +     struct efi_configuration_table *cfgtab;
> > +     size_t table_size = sizeof(struct efi_system_table) +
> > +                         nr_cfgtab * sizeof(struct efi_configuration_table);
> > +
> > +     systab = memalign(SZ_64K, table_size);
> 
> Please add a comment on why 64K alignment is required.

This is a mistake, we wrongly assumed the system table must be aligned
to page size, and on LoongArch the least common multiple among possible
page sizes (4K, 16K, 64K) is 64K. However such requirements do not
really exist at all, and I'll remove them in v3.

> > diff --git a/arch/loongarch/lib/bootm.c b/arch/loongarch/lib/bootm.c
> > @@ -0,0 +1,163 @@
> > +static void boot_jump_linux(struct bootm_headers *images, int flag)
> > +{
> > +     void (*kernel)(ulong efi_boot, char *argc, void *dtb);
> > +     int fake = (flag & BOOTM_STATE_OS_FAKE_GO);
> > +
> > +     kernel = (void (*)(ulong efi_boot, char *argc, void *dtb))images->ep;
> 
> The second argument is the kernel command line, not argc - please
> rename to cmdline. The third is the fake systab pointer, not a raw
> dtb; please rename that too.
> 
> > diff --git a/arch/loongarch/lib/bootm.c b/arch/loongarch/lib/bootm.c
> > @@ -0,0 +1,163 @@
> > +     if (!fake) {
> > +             if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len)
> > +                     kernel(0, NULL, (void *)images->kbd->bi_boot_params);
> > +     }
> 
> Per the LoongArch booting spec, a1 is the kernel command line.
> Passing NULL means the bootargs env var never reaches the kernel
> via that path - it will only be seen in /chosen/bootargs
> 
> So don't you wan to pass images->cmdline_start here?

Yes, thanks for catching this. It doesn't cause any issues yet, since
LoongArch platform kernel code currently appends the cmdline passed in
a1 to /chosen/bootargs.

> Also, if OF_LIBFDT is disabled or ft_len is zero we drop through
> without jumping *after* announce_and_cleanup() has torn devices
> down. Please panic() / hang() in that case rather than silently return.

boot_prep_linux() does a check against OF_LIBFDT and ft_len, too, and
might emit an error and hang. I'm not very familiar with bootm, could
boot_jump_linux be called with BOOTM_STATE_OS_GO set but
BOOTM_STATE_OS_PREP unset, which is the only possible case of dropping
through this if? This doesn't really make sense to me at the first
glance...

But anyway, panic/hang() here makes the intention more clear, and I'm
willing to add one here.

> > diff --git a/arch/loongarch/lib/image.c b/arch/loongarch/lib/image.c
> > @@ -0,0 +1,66 @@
> > +struct linux_image_h {
> > +     uint32_t        code0;                  /* Executable code */
> > +     uint32_t        code1;                  /* Executable code */
> > +     uint64_t        kernel_entry;   /* Kernel entry point */
> > +     uint64_t        image_size;             /* Effective Image size */
> > +     uint64_t        load_offset;    /* load offset */
> > +     uint64_t        res1;                   /* reserved */
> > +     uint64_t        res2;                   /* reserved */
> > +     uint64_t        res3;                   /* reserved */
> > +     uint32_t        magic;                  /* Magic number */
> > +     uint32_t        pe_header;              /* Offset to the PE header */
> > +};
> 
> Comment tabs after kernel_entry/image_size/load_offset are
> inconsistent with the first two members - please tidy so the
> comments line up. Also use u32/u64 for consistency with the rest
> of U-Boot.
> 
> I have to wonder whether it would be OK to share this struct with
> RISC-V in a header file?

RISC-V header is compatible with the AArch64 one, but not the LoongArch
one, here's RISC-V's definition,

u32 code0;                /* Executable code */
u32 code1;                /* Executable code */
u64 text_offset;          /* Image load offset, little endian */
u64 image_size;           /* Effective Image size, little endian */
u64 flags;                /* kernel flags, little endian */
u32 version;              /* Version of this header */
u32 res1 = 0;             /* Reserved */
u64 res2 = 0;             /* Reserved */
u64 magic = 0x5643534952; /* Magic number, little endian, "RISCV" */
u32 magic2 = 0x05435352;  /* Magic number 2, little endian, "RSC\x05" */
u32 res3;                 /* Reserved for PE COFF offset */

LoongArch's header has an additional kernel_entry field, but no flags
field. It also has only one magic number. So I don't think merging
RISC-V and LoongArch's definitions make much sense.

> > diff --git a/arch/loongarch/lib/image.c b/arch/loongarch/lib/image.c
> > @@ -0,0 +1,66 @@
> > +     /* To workaround kernel supplying DMW based virtual address */
> > +     ep_phys = TO_PHYS(lhdr->kernel_entry);
> > +     *ep = *relocated_addr + (ep_phys - lhdr->load_offset);
> 
> Please expand this comment - from context it looks like kernel_entry
> is a DMW virtual address, and you are converting to a physical
> offset within the image and re-basing onto the relocated load
> address. Spell that out, with a pointer to the relevant paragraph
> of the booting spec.

DMW is abbreviated from Direct Mapping Window. Both load_offset and
kernel_entry should be physical addresses, respectively represent the
expected kernel image loading address and entry point. However, Linux
kernel before v6.10 wrongly fills in a DMW virtual address in
kernel_entry, so we must convert it back to physical address to
calculate the entrypoint offset relative to the image start.

On loongarch64, if an virtual address matches any configured DMW,
it would be translated to the physical address represented by its lowest
48 bits, and TO_PHYS() does the same thing.

Here's the Linux commit fixing the issue[1], and I'll make the comment
more clear in the next version.

> Regards,
> Simon

Regards,
Yao Zi

[1]: https://github.com/torvalds/linux/commit/beb2800074c1