Re: [PULL 25/33] tests/acceptance: Add a test for the N800 and N810 arm machines

[Igor Mammedov]

On Fri, 23 Oct 2020 19:39:16 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 10/23/20 5:43 PM, Igor Mammedov wrote:
> > On Mon, 19 Oct 2020 11:43:13 +0200
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> >>>>> FYI this test is failing:
> >>>>>
> >>>>> qemu-system-arm: kernel 'meego-arm-n8x0-1.0.80.20100712.1431-vml
> >>>>> inuz-2.6.35~rc4-129.1-n8x0' is too large to fit in RAM (kernel size
> >>>>> 1964608,
> >>>>> RAM size 0)  
> >>>
> >>> FWIW:
> >>>
> >>> 7998beb9c2e280f0b7424223747941f106e2e854 is the first bad commit
> >>> commit 7998beb9c2e280f0b7424223747941f106e2e854
> >>> Author: Igor Mammedov <imammedo@redhat.com>
> >>> Date:   Wed Feb 19 11:08:59 2020 -0500
> >>>
> >>>       arm/nseries: use memdev for RAM
> >>>
> >>>       memory_region_allocate_system_memory() API is going away, so
> >>>       replace it with memdev allocated MemoryRegion. The later is
> >>>       initialized by generic code, so board only needs to opt in
> >>>       to memdev scheme by providing
> >>>         MachineClass::default_ram_id
> >>>       and using MachineState::ram instead of manually initializing
> >>>       RAM memory region.
> >>>
> >>>       PS:
> >>>        while at it add check for user supplied RAM size and error
> >>>        out if it mismatches board expected value.
> >>>
> >>>       Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>       Reviewed-by: Andrew Jones <drjones@redhat.com>
> >>>       Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >>>       Message-Id: <20200219160953.13771-26-imammedo@redhat.com>  
> >>
> >> This fixes the issue:
> >>  
> >> -- >8 --  
> >> diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
> >> index e48092ca047..76fd7fe9854 100644
> >> --- a/hw/arm/nseries.c
> >> +++ b/hw/arm/nseries.c
> >> @@ -1318,6 +1318,7 @@ static void n8x0_init(MachineState *machine,
> >>            g_free(sz);
> >>            exit(EXIT_FAILURE);
> >>        }
> >> +    binfo->ram_size = machine->ram_size;
> >>
> >>        memory_region_add_subregion(get_system_memory(), OMAP2_Q2_BASE,
> >>                                    machine->ram);  
> > 
> > we really should replace binfo->ram_size with machine->ram_size to avoid
> > duplicating the same data, but as a quick fix this should fix issue.  
> 
> Hmm this is the 'ARM kernel loader' API in "arm/boot.h":
> 
> struct arm_boot_info {
>      uint64_t ram_size;
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *initrd_filename;
>      const char *dtb_filename;
> 
> and:
> 
>    void (*write_secondary_boot)(ARMCPU *cpu,
>                                 const struct arm_boot_info *info);
>    void (*secondary_cpu_reset_hook)(ARMCPU *cpu,
>                                     const struct arm_boot_info *info);
> 
> Are you saying arm_boot_info should hold a pointer to MachineState*
> instead of duplicating?

yep, some parts of it (fdt related) already use MachineState* so it's
complete rewrite. The same probably applies to the fields you've just
quoted.

>