Am 22. Februar 2023 02:38:04 UTC schrieb Xiaoyao Li <xiaoyao...@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) {
>
