Re: [PATCH v4 07/11] xen/riscv: add Linux kernel loading support

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/6/26 2:45 PM, Jan Beulich wrote:
> On 06.05.2026 13:57, Oleksii Kurochko wrote:
> > On 5/4/26 4:05 PM, Jan Beulich wrote:
> > > On 28.04.2026 16:33, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/kernel.c
> > > > @@ -0,0 +1,242 @@
> > > > +/* 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);
> > > Why would modules need to be this strongly aligned?
> > No specific reason except to be aligned with similar alignment below, it
> > could be lesser (PAGE_SIZE or even just unsigned long aligned) or even
> > dropped, I think. It was just easier then to calculate aligned
> > addresses. But I don't see any big issue to have such alignments except
> > maybe that it will waste some memory.
> Or result in there not being enough memory to hold everything.
Do you prefer than not to have alignment at all?

> > > > +    /*
> > > > +     * Place modules as high in RAM as possible, scanning banks from
> > > > +     * last to first so that the end of the last bank is preferred.
> > > > +     */
> > > > +    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);
> > > Same question here.
> > I used KERNEL_LOAD_ADDR_ALIGNMENT to be sure that big page tables be
> > potentially used in page table.
> I fear I'm lost. All the modules are temporary entities, aren't they?
They are temporary entities but they should be copied to guest memory, 
right?
So ioremap() should be called for paddr where module is located and so 
at least less cycles will be needed to add entries to Xen page tables.
I don't know if it makes sense to have such type of optimizations. If 
not then probably we don't need alignment here too. I don't see at the 
moment any alignment requirements for initrd and dtb.
The only theoretical reason why at least for dtb we need requirement is 
that what Arm mentioned in its booting.rst:
The device tree blob (dtb) must be placed on an 8-byte boundary and must
not exceed 2 megabytes in size. Since the dtb will be mapped cacheable
using blocks of up to 2 megabytes in size, it must not be placed within
any 2M region which must be mapped with any specific attributes.

It likely should be true for RISC-V as I assume this part was copied 
from Arm port. On other side it doesn't mentioned explicitly in boot.rst 
of RISC-V in LK.
> > > > +        if ( (modbase < ROUNDUP(kernend, KERNEL_LOAD_ADDR_ALIGNMENT)) &&
> > > > +             (modbase + modsize > kernbase) )
> > > > +        {
> > > > +            modbase = ROUNDDOWN(kernbase - modsize, 
> > > > KERNEL_LOAD_ADDR_ALIGNMENT);
> > > What prevents this subtraction from underflowing?
> > I will put the following check at the start of the place_modules() function:
> > if ( kernbase < modsize )
> >      panic("Underflow could happen between kernbase and modsize\n");
> Wait - why would this be a legitimate condition to panic?
It is legitimate to panic() as common API which leads to place_module() 
has void in its return type (what should be changed in future, I have 
this in TODO) and so if something is going wrong in place_module() there 
is not better option except panic() for now.
But generally i think it was too much to panic and it would be just 
better to put:
  if ( kernbase < modsize )
      continue;
above modbase = ROUNDDOWN(...) so it will just put modules in different 
bank.
Thanks.

~ Oleksii