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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>> +        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