Re: [PATCH v4 6/9] hw/i386/pc: Initialize ram_memory variable directly

[Bernhard Beschow]

Am 22. Februar 2023 02:38:04 UTC schrieb Xiaoyao Li <xiaoyao.li@intel.com>:
>On 2/14/2023 12:20 AM, Bernhard Beschow wrote:
>> Going through pc_memory_init() seems quite complicated for a simple
>> assignment.
>> 
>
>...
>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 5bde4533cc..00ba725656 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -143,6 +143,7 @@ static void pc_init1(MachineState *machine,
>>       if (xen_enabled()) {
>>           xen_hvm_init_pc(pcms, &ram_memory);

In Xen mode, ram_memory is initialized here and machine->ram isn't used at all.

>>       } else {
>> +        ram_memory = machine->ram;

The idea of moving the assignment here is to make the code more symmetric to 
Xen and to establish the invariant of ram_memory being initialized after this 
if-else block. IOW one shouldn't need to use machine->ram directly after this 
point.

>>           if (!pcms->max_ram_below_4g) {
>>               pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
>>           }
>> @@ -205,8 +206,7 @@ static void pc_init1(MachineState *machine,
>>         /* allocate ram and load rom/bios */
>>       if (!xen_enabled()) {
>> -        pc_memory_init(pcms, system_memory,
>> -                       rom_memory, &ram_memory, hole64_size);
>
>IMHO, it seems more proper to put
>
>+        ram_memory = machine->ram;
>
>here rather than above.

This patch allowed for further cleanup (establishing above invariant) which I 
inlined for simplicity.

Do you still see issues here?

Thanks,
Bernhard
>
>> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>>       } else {
>>           pc_system_flash_cleanup_unused(pcms);
>>           if (machine->kernel_filename != NULL) {
>