Re: [PATCH v3 05/12] xen/riscv: add kernel loading support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.04.2026 17:54, Oleksii Kurochko wrote:
> --- 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

I.e. this is entirely Linux-centric? If so, maybe the patch subject should
then reflect this?

> --- /dev/null
> +++ b/xen/arch/riscv/kernel.c
> @@ -0,0 +1,230 @@
> +/* 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 struct membanks *banks = kernel_info_get_mem_const(info);
> +    const paddr_t initrd_len = ROUNDUP(mod ? mod->size : 0,
> +                                       KERNEL_LOAD_ADDR_ALIGNMENT);
> +    const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt),
> +                                    KERNEL_LOAD_ADDR_ALIGNMENT);
> +    const paddr_t modsize = initrd_len + dtb_len;
> +    int bi;

Please can variables used for array indexing be of unsigned types? The use ...

> +    BUG_ON(modsize < initrd_len);
> +
> +    /*
> +     * Place modules as high in RAM as possible, scanning banks from
> +     * last to first so that the end of the last bank is preferred.
> +     */
> +    for ( bi = banks->nr_banks - 1; bi >= 0; bi-- )

... here can easily be replaced:

    for ( bi = banks->nr_banks; bi-- > 0; )

Or you could have

    unsigned int bi = banks->nr_banks;
    ...
    while ( bi-- > 0 )

.

> +    {
> +        const struct membank *bank = &banks->bank[bi];
> +        const paddr_t bank_end = bank->start + bank->size;
> +        paddr_t modbase;
> +
> +        if ( modsize > bank->size )
> +            continue;
> +
> +        modbase = ROUNDDOWN(bank_end - modsize, KERNEL_LOAD_ADDR_ALIGNMENT);
> +
> +        if ( modbase < bank->start )
> +            continue;
> +
> +        /*
> +         * If the kernel resides in this bank, ensure modules do not
> +         * overlap with it.
> +         */
> +        if ( (kernbase >= bank->start) && (kernbase < bank_end) &&
> +             (modbase < ROUNDUP(kernend, KERNEL_LOAD_ADDR_ALIGNMENT)) &&
> +             (modbase + modsize > kernbase) )
> +            continue;

Can't this be had with only two comparisons? Same bank or not doesn't really
matter - if it's different banks, there'll be no overlap anyway. So all you
need here is that the module range doesn't overlap the kernel range, entirely
independent of the bank.

What is dependent on the bank is that the bank may fit both kernel and module
even if there is an overlap as per your current calculation: You may be able
to place the module below the kernel if it doesn't fit above.

> +static paddr_t __init kernel_image_place(struct kernel_info *info)
> +{
> +    paddr_t load_addr = INVALID_PADDR;
> +    uint64_t image_size = info->image.image_size ?: info->image.len;
> +    const struct membanks *banks = kernel_info_get_mem_const(info);
> +    unsigned int nr_banks = banks->nr_banks;
> +    unsigned int bi;
> +
> +    dprintk(XENLOG_DEBUG, "nr_banks(%u)\n", nr_banks);
> +
> +    /*
> +     * 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) )
> +    {
> +        for ( bi = 0; bi != nr_banks; bi++ )
> +        {
> +            const struct membank *bank = &banks->bank[bi];
> +            paddr_t bank_start = bank->start;
> +            /*
> +             * According to boot.rst kernel load address should be properly
> +             * aligned:
> +             *   https://docs.kernel.org/arch/riscv/boot.html#kernel-location
> +             *
> +             * As Image in this case is PIC we can ignore
> +             * info->image.text_offset.
> +             */
> +            paddr_t aligned_start = ROUNDUP(bank_start, 
> KERNEL_LOAD_ADDR_ALIGNMENT);
> +            paddr_t bank_end = bank_start + bank->size;
> +            paddr_t bank_size;
> +
> +            if ( aligned_start > bank_end )
> +                continue;
> +
> +            bank_size = bank_end - aligned_start;
> +
> +            dprintk(XENLOG_DEBUG, "bank[%u].start=%"PRIpaddr"\n", bi, 
> bank->start);
> +
> +            if ( image_size <= bank_size )
> +            {
> +                load_addr = aligned_start;
> +                break;
> +            }
> +        }
> +    }
> +    else
> +    {
> +        load_addr = info->image.start + info->image.text_offset;

Why does stuff ahead of text_offset not need loading?

> +        WARN_ON(!IS_ALIGNED(load_addr, KERNEL_LOAD_ADDR_ALIGNMENT));
> +
> +        for ( bi = 0; bi != nr_banks; bi++ )
> +        {
> +            const struct membank *bank = &banks->bank[bi];
> +            paddr_t bank_start = bank->start;
> +            paddr_t bank_end = bank_start + bank->size;
> +
> +            if ( (load_addr >= bank_start) && (load_addr < bank_end) &&
> +                 (bank_end - load_addr) >= image_size )

Do we have to fear overflow? (If so, shouldn't such an image be rejected
rather than an attempt being made to place it?) If not, simply:

            if ( (load_addr >= bank_start) && 
                 (load_addr + image_size <= bank_end) )

Also, does image_size really only cover space starting from .text_offset,
rather than from .start?

> +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;
> +    paddr_t effective_size = info->image.image_size ?: len;
> +    void *kernel;
> +
> +    place_modules(info, load_addr, load_addr + effective_size);
> +
> +    printk("Loading Image from %"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr"\n",
> +            paddr, load_addr, load_addr + effective_size);

As on earlier occasions: Please represent ranges as mathematical ones, to
disambiguate whether the bounds (the upper one in particular) are inclusive
or exclusive.

> +int __init kernel_image_probe(struct kernel_info *info, paddr_t addr,
> +                              paddr_t size)
> +{
> +#ifdef CONFIG_RISCV_64
> +    return kernel_image64_probe(info, addr, size);
> +#else
> +    return -EOPNOTSUPP;

Better #error, as you have it elsewhere (iirc)?

Jan