Re: [PATCH v2 05/11] xen/riscv: add kernel loading support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2026 17:29, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -8,6 +8,7 @@ obj-y += guestcopy.o
>  obj-y += imsic.o
>  obj-y += intc.o
>  obj-y += irq.o
> +obj-y += kernel.o

kernel.init.o, like Arm has it?

> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -151,6 +151,19 @@
>  extern unsigned long phys_offset; /* = load_start - XEN_VIRT_START */
>  #endif
>  
> +/*
> + * KERNEL_LOAD_ADDR_ALIGNMENT is defined based on paragraph of
> + * "Kernel location" of boot.rst:
> + * https://docs.kernel.org/arch/riscv/boot.html#kernel-location
> + */
> +#if defined(CONFIG_RISCV_32)
> +#define KERNEL_LOAD_ADDR_ALIGNMENT MB(4)
> +#elif defined(CONFIG_RISCV_64)
> +#define KERNEL_LOAD_ADDR_ALIGNMENT MB(2)
> +#else
> +#error "Define KERNEL_LOAD_ADDR_ALIGNMENT"
> +#endif

But that's Linux-specific. You want to be able to loader other OS kernels,
I suppose? The needed alignment should be a property of the kernel image,
suitably conveyed to the loader.

Is Arm similarly capable of loading only Linux images? What about in
particular XTF?

> --- /dev/null
> +++ b/xen/arch/riscv/kernel.c
> @@ -0,0 +1,158 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/bug.h>
> +#include <xen/compiler.h>
> +#include <xen/errno.h>
> +#include <xen/fdt-kernel.h>
> +#include <xen/guest_access.h>
> +#include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/mm.h>
> +#include <xen/types.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/setup.h>
> +
> +#define IMAGE64_MAGIC_V2 0x05435352 /* Magic number 2, le, "RSC\x05" */
> +
> +static void __init place_modules(struct kernel_info *info, paddr_t kernbase,
> +                                 paddr_t kernend)
> +{
> +    const struct boot_module *mod = info->bd.initrd;
> +
> +    const paddr_t initrd_len = ROUNDUP(mod ? mod->size : 0, MB(2));
> +    const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
> +    const paddr_t modsize = initrd_len + dtb_len;
> +
> +    const paddr_t ramsize = info->mem.bank[0].size;
> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
> +
> +    if ( modsize + kernsize > ramsize )
> +        panic("Not enough memory in the first bank for the 
> kernel+dtb+initrd\n");
> +
> +    info->dtb_paddr = ROUNDUP(kernend, MB(2));
> +
> +    info->initrd_paddr = info->dtb_paddr + dtb_len;
> +}

Where are all of the MB(2) coming from in here? Do they mean to be
KERNEL_LOAD_ADDR_ALIGNMENT?

Also, how come all of this is limited to the first memory bank?

> +static paddr_t __init kernel_image_place(struct kernel_info *info)
> +{
> +    paddr_t load_addr;
> +
> +    /*
> +     * At the moment, RISC-V's Linux kernel should be always position
> +     * independent based on "Per-MMU execution" of boot.rst:
> +     *   https://docs.kernel.org/arch/riscv/boot.html#pre-mmu-execution
> +     *
> +     * But just for the case when RISC-V's Linux kernel isn't position
> +     * independent it is needed to take load address from
> +     * info->image.start.
> +     *
> +     * If `start` is zero, the Image is position independent. */
> +    if ( likely(!info->image.start) )
> +        /*
> +         * According to boot.rst kernel load address should be properly
> +         * aligned:
> +         *   https://docs.kernel.org/arch/riscv/boot.html#kernel-location
> +         */
> +        load_addr = ROUNDUP(info->mem.bank[0].start, 
> KERNEL_LOAD_ADDR_ALIGNMENT);
> +    else
> +        load_addr = info->image.start;
> +
> +    return load_addr;
> +}

*info doesn't look to be altered here, so likely the parameter wants to
be pointer-to-const.

> +static void __init kernel_image_load(struct kernel_info *info)
> +{
> +    int rc;
> +    paddr_t load_addr = kernel_image_place(info);
> +    paddr_t paddr = info->image.kernel_addr;
> +    paddr_t len = info->image.len;
> +    void *kernel;
> +
> +    info->entry = load_addr;

What if this is outside of memory bank 0 (as is possible when
info->image.start is non-zero).

> +    place_modules(info, load_addr, load_addr + len);
> +
> +    printk("Loading Image from %"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr"\n",
> +            paddr, load_addr, load_addr + len);
> +
> +    kernel = ioremap_wc(paddr, len);

ioremap_cache()?

> +/* Check if the image is a 64-bit Image */
> +static int __init kernel_image64_probe(struct kernel_info *info,
> +                                       paddr_t addr, paddr_t size)
> +{
> +    /* riscv/boot-image-header.rst */
> +    struct {
> +        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;              /* Reserved */
> +        u64 res2;              /* Reserved */
> +        u64 magic;        /* Deprecated: Magic number, little endian, 
> "RISCV" */
> +        u32 magic2;       /* Magic number 2, little endian, "RSC\x05" */
> +        u32 res3;              /* Reserved for PE COFF offset */

uint<N>_t throughout, please. And no use of hard tabs.

> +    } image;
> +    uint64_t start, end;
> +
> +    if ( size < sizeof(image) )
> +        return -EINVAL;
> +
> +    copy_from_paddr(&image, addr, sizeof(image));
> +
> +    /* Magic v1 is deprecated and may be removed.  Only use v2 */
> +    if ( image.magic2 != IMAGE64_MAGIC_V2 )
> +        return -EINVAL;

This doesn't look to be endian-ness-agnostic.

> +    /* Currently there is no length in the header, so just use the size */
> +    start = 0;
> +    end = size;

What's image_size then?

> +    /*
> +     * Given the above this check is a bit pointless, but leave it
> +     * here in case someone adds a length field in the future.
> +     */
> +    if ( (end - start) > size )
> +        return -EINVAL;
> +
> +    info->image.kernel_addr = addr;
> +    info->image.len = end - start;
> +    info->image.text_offset = image.text_offset;

This again doesn't look to be endian-ness-agnostic.

Jan