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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2026 15:43, Oleksii Kurochko wrote:
> 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?

Some minimal alignment (like to machine word size) may be helpful, for
copying to be more efficient. There may also be reasons to use page
alignment (e.g. if the pages were to be assigned directly to the
domains, without any copying, as we do for PV Dom0 under certain
conditions on x86). That in turn may even justify super-page
alignment.

>>>>> +    /*
>>>>> +     * 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.

Okay, that would be a small win in time for perhaps a boot failure when
the modules would fit in memory if they weren't this heavily aligned.
You're judgement, but please whatever higher-than-expected alignment
you decide to use, please comment upon this.

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

Feels like you're mixing two things here. I didn't ask whether panic()
was appropriate to use here, but whether the condition is one upon
which panic()ing is the only option (right now). And voila, ...

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

... looks like the situation can be handled without panic().

Jan