Re: [PATCH v3 39/43] hw/loongarch: Add LoongArch load elf function.

[Richard Henderson]

On 4/29/22 05:07, Xiaojuan Yang wrote:
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   hw/loongarch/loongson3.c    | 66 +++++++++++++++++++++++++++++++++++--
>   include/hw/loongarch/virt.h |  9 +++++
>   target/loongarch/cpu.h      |  2 ++
>   3 files changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/hw/loongarch/loongson3.c b/hw/loongarch/loongson3.c
> index 7029d8c8b8..f9ee024f63 100644
> --- a/hw/loongarch/loongson3.c
> +++ b/hw/loongarch/loongson3.c
> @@ -19,6 +19,8 @@
>   #include "exec/address-spaces.h"
>   #include "hw/irq.h"
>   #include "net/net.h"
> +#include "hw/loader.h"
> +#include "elf.h"
>   #include "hw/intc/loongarch_ipi.h"
>   #include "hw/intc/loongarch_extioi.h"
>   #include "hw/intc/loongarch_pch_pic.h"
> @@ -29,6 +31,36 @@
>   
>   #include "target/loongarch/cpu.h"
>   
> +static struct _loaderparams {
> +    unsigned long ram_size;
> +    const char *kernel_filename;
> +} loaderparams;

Never use "unsigned long", only "uint{32,64}_t" or "size_t".
Otherwise you don't know what the host is going to give you.

> +static int64_t load_kernel_info(void)
> +{
> +    int64_t kernel_entry, kernel_low, kernel_high;

Why are you using signed values here,

> +    long kernel_size;
> +
> +    kernel_size = load_elf(loaderparams.kernel_filename, NULL,
> +                           cpu_loongarch_virt_to_phys, NULL,
> +                           (uint64_t *)&kernel_entry, (uint64_t *)&kernel_low,
> +                           (uint64_t *)&kernel_high, NULL, 0,

and casting them?  Oh, and kernel_size must be ssize_t.

> @@ -237,22 +283,38 @@ static void loongarch_init(MachineState *machine)
>           cpu_create(machine->cpu_type);
>       }
>   
> +    if (ram_size < 1 * GiB) {
> +        error_report("ram_size must be greater than 1G.");
> +        exit(1);
> +    }

Why is this here?  It's certainly not related to load_elf.


r~