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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/22/26 3:54 PM, Jan Beulich wrote:
> On 22.04.2026 15:47, Oleksii Kurochko wrote:
> > 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?
> I don't know, but the adding in of .text_offset looks questionable to me.
> I simply don't know why such offsetting would be wanted / needed.
In the <linux>/Documentation/arch/riscv/boot-image-header.rst:

This header format is compliant with PE/COFF header and largely inspired 
from ARM64 header. Thus, both ARM64 & RISC-V header can be combined into 
one common header in future.
Than if to look at the comment near text_offset it is mentioned that:
/* Image load offset, little endian */

So it is basically Image load offset and that is why I expect it should 
be added to calculation of load_addr.
Considering that boot image header is based on Arm64 I assume that an 
explanation regarding text_offset for Arm64 
(<linux>/Documentation/arch/arm64/booting.rst:) should be true for RISC-V:
```
The Image must be placed text_offset bytes from a 2MB aligned base
address anywhere in usable system RAM and called there. The region
between the 2 MB aligned base address and the start of the image has no
special significance to the kernel, and may be used for other purposes.
At least image_size bytes from the start of the image must be free for
use by the kernel.
```

~ Oleksii