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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/21/26 10:57 AM, Jan Beulich wrote:
> 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?
At least. for now - yes. I will add Linux kernel to the patch subject.

> > --- /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 )
>
> .
I will use while() form.

> > +    {
> > +        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.
I will drop the first check and update the comment:
        /*
         * If modules would overlap the kernel, try placing them below it.
         */
        if ( (modbase < ROUNDUP(kernend, KERNEL_LOAD_ADDR_ALIGNMENT)) &&
             (modbase + modsize > kernbase) )
        {
            modbase = ROUNDDOWN(kernbase - modsize, 
KERNEL_LOAD_ADDR_ALIGNMENT);
            if ( modbase < bank->start )
                continue;
        }

> > +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?
Here we just calculating only a place where kernel will be loaded. The 
full kernel image will be loaded in kernel_image_load().
> > +        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:
Just for a case. As a user may control load_addr and image_size it could 
be some combination which will lead to overflow here.
>              if ( (load_addr >= bank_start) &&
>                   (load_addr + image_size <= bank_end) )
I will add the following:
        /*
         * Reject a malformed image before the loop to avoid wrapping
         * load_addr + image_size in the per-bank check below.
         *
         * image_size covers the kernel from _start (placed at load_addr =
         * start + text_offset) through _end.  The alignment gap
         * [start, load_addr) is padding and need not lie within a bank.
         */
        if ( image_size > (paddr_t)-1 - load_addr )
            bi = nr_banks;
        else
            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 + image_size <= bank_end) )
                    break;
            }

> Also, does image_size really only cover space starting from .text_offset,
> rather than from .start?
image_size covers total memory the kernel occupies at runtime.

> > +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.
I will change to "... [%"PRIpaddr",%"PRIpaddr")\n".

> > +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)?
Makes sense use:

#   error "Only 64-bit RISC-V is supported"

Thanks.

~ Oleksii