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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/22/26 1:57 PM, Jan Beulich wrote:
> On 22.04.2026 13:45, Oleksii Kurochko wrote:
> > On 4/21/26 10:57 AM, Jan Beulich wrote:
> > > On 10.04.2026 17:54, Oleksii Kurochko wrote:
> > > > +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().
> Okay, but if you calculate an address where the full image won't fit,
> how are things going to work?
If the full image won't fit than the necessary bank won't be found in 
for() loop below and so this kernel will be rejected.
I expect that in the case when info->image.start is not 0 (so isn't 
Image isn't PIC) Image want to be specifically load to info->image.start 
+ info->image.text_offset. Is it wrong statement?
~ Oleksii

> > > > +        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;
> >               }
> Please consider getting away without "else" (and hence with one level
> less of indentation):
>
>           bi = image_size <= (paddr_t)-1 - load_addr ? 0 : nr_banks;
>           for ( ; bi != nr_banks; bi++ )
>           ...
>
> > > 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.
> Which emphasizes the remark further up.
>
> Jan