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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

>>> +/* Check if the image is a 64-bit Image */
>>> +static int __init kernel_image64_probe(struct kernel_info *info,
>>> +                                       paddr_t addr, paddr_t size)
>>> +{
>>> +    /* 
>>> https://www.kernel.org/doc/Documentation/riscv/boot-image-header.rst */
>>> +    struct {
>>> +        uint32_t code0;         /* Executable code */
>>> +        uint32_t code1;         /* Executable code */
>>> +        uint64_t text_offset;   /* Image load offset, little endian */
>>> +        uint64_t image_size;    /* Effective Image size, little endian */
>>> +        uint64_t flags;         /* kernel flags, little endian */
>>> +        uint32_t version;       /* Version of this header */
>>> +        uint32_t res1;          /* Reserved */
>>> +        uint64_t res2;          /* Reserved */
>>> +        uint64_t magic;         /* Deprecated: Magic number, little 
>>> endian, "RISCV" */
>>> +        uint32_t magic2;        /* Magic number 2, little endian, 
>>> "RSC\x05" */
>>> +        uint32_t res3;          /* Reserved for PE COFF offset */
>>> +    } image;
>>> +    uint64_t effective_size;
>>> +
>>> +    if ( size < sizeof(image) )
>>> +        return -EINVAL;
>>> +
>>> +    copy_from_paddr(&image, addr, sizeof(image));
>>> +
>>> +    /* Magic v1 is deprecated and may be removed.  Only use v2 */
>>> +    if ( le32_to_cpu(image.magic2) != IMAGE64_MAGIC_V2 )
>>> +        return -EINVAL;
>>> +
>>> +    effective_size = le64_to_cpu(image.image_size);
>>> +
>>> +    if ( effective_size && size > effective_size )
>>> +        return -EINVAL;
>>
>> Is the rhs of the && the wrong way round? If effective_size > size,
>> aren't you in trouble? Question of course is what "effective" really
>> means. Yet in any event it seems dubious to me that effective_size <
>> size would really be a problem. IOW this will want commenting upon
>> if the check is to stay.
>>
>> Actually ...
>>
>>> +    info->image.kernel_addr = addr;
>>> +    /* Actual size in the binary file */
>>> +    info->image.len = size;
>>> +    /* Total memory the kernel occupies at runtime */
>>> +    info->image.image_size = effective_size;
>>
>> ... this looks to suggest something .bss-like.
> 
> Yes, effective_size it is size which included .bss.
> 
> size it of LK after decompression of Image.gz and it doesn't include 
> .bss so it should be lesser then effective_size.
> 
> I don't think that I am in trouble that effective_size is bigger then 
> size if we allocate enough space in memory effective_size is fine to be 
> bigger.
> 
> It is a good question if effective_size < size is a problem. I think it
> isn't but could it be really happen?

A kernel image (file) could have data appended to it, e.g. a certificate.
With only small .bss that certificate could end up larger than the .bss
size, and hence effective_size < size.

> I think that I am okay to drop that part of if().

Please do, unless there is a(nother) reason to have it.

Jan