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

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Igor Mammedov <imammedo@redhat.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	qemu-devel@nongnu.org
Subject: Re: [PULL 25/33] tests/acceptance: Add a test for the N800 and N810 arm machines
Date: Fri, 23 Oct 2020 19:39:16 +0200	[thread overview]
Message-ID: <c80350d2-3591-6541-abf3-960b69cedf7d@amsat.org> (raw)
In-Reply-To: <20201023174351.31838a86@redhat.com>

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?