diff mbox series

[v3,6/9] hw/i386/pc: Initialize ram_memory variable directly

Message ID 20230204151027.39007-7-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series PC cleanups | expand

Commit Message

Bernhard Beschow Feb. 4, 2023, 3:10 p.m. UTC 
Going through pc_memory_init() seems quite complicated for a simple
assignment.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/pc.h | 1 -
 hw/i386/pc.c         | 2 --
 hw/i386/pc_piix.c    | 4 ++--
 hw/i386/pc_q35.c     | 5 ++---
 4 files changed, 4 insertions(+), 8 deletions(-)

Comments

BALATON Zoltan Feb. 4, 2023, 3:26 p.m. UTC | #1 
On Sat, 4 Feb 2023, Bernhard Beschow wrote:
> Going through pc_memory_init() seems quite complicated for a simple
> assignment.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/i386/pc.h | 1 -
> hw/i386/pc.c         | 2 --
> hw/i386/pc_piix.c    | 4 ++--
> hw/i386/pc_q35.c     | 5 ++---
> 4 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 66e3d059ef..b60b95921b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -162,7 +162,6 @@ void xen_load_linux(PCMachineState *pcms);
> void pc_memory_init(PCMachineState *pcms,
>                     MemoryRegion *system_memory,
>                     MemoryRegion *rom_memory,
> -                    MemoryRegion **ram_memory,
>                     uint64_t pci_hole64_size);
> uint64_t pc_pci_hole64_start(void);
> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6e592bd969..8898cc9961 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -936,7 +936,6 @@ static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
> void pc_memory_init(PCMachineState *pcms,
>                     MemoryRegion *system_memory,
>                     MemoryRegion *rom_memory,
> -                    MemoryRegion **ram_memory,
>                     uint64_t pci_hole64_size)
> {
>     int linux_boot, i;
> @@ -994,7 +993,6 @@ void pc_memory_init(PCMachineState *pcms,
>      * Split single memory region and use aliases to address portions of it,
>      * done for backwards compatibility with older qemus.
>      */
> -    *ram_memory = machine->ram;
>     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
>     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", machine->ram,
>                              0, x86ms->below_4g_mem_size);
> 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);
>     } else {
> +        ram_memory = machine->ram;

Maybe you could just replace the few places it's used with machine->ram 
directly and get rid of the local variable. There seems to be no advantage 
storing it in a local just to use it once (in q35 below) or twice in 
pc-piix. The local name is not even that much shorter so I don't see a 
reason to have it in the first place,

Regards,
BALATON Zoltan

>         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);
> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>     } else {
>         pc_system_flash_cleanup_unused(pcms);
>         if (machine->kernel_filename != NULL) {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 8253b49296..88f0981f50 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -129,7 +129,7 @@ static void pc_q35_init(MachineState *machine)
>     MemoryRegion *system_io = get_system_io();
>     MemoryRegion *pci_memory;
>     MemoryRegion *rom_memory;
> -    MemoryRegion *ram_memory;
> +    MemoryRegion *ram_memory = machine->ram;
>     GSIState *gsi_state;
>     ISABus *isa_bus;
>     int i;
> @@ -216,8 +216,7 @@ static void pc_q35_init(MachineState *machine)
>     }
>
>     /* allocate ram and load rom/bios */
> -    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
> -                   pci_hole64_size);
> +    pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
>
>     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
>     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
>
Bernhard Beschow Feb. 6, 2023, 12:07 a.m. UTC | #2 
Am 4. Februar 2023 15:26:13 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 4 Feb 2023, Bernhard Beschow wrote:
>> Going through pc_memory_init() seems quite complicated for a simple
>> assignment.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/hw/i386/pc.h | 1 -
>> hw/i386/pc.c         | 2 --
>> hw/i386/pc_piix.c    | 4 ++--
>> hw/i386/pc_q35.c     | 5 ++---
>> 4 files changed, 4 insertions(+), 8 deletions(-)
>> 
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 66e3d059ef..b60b95921b 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -162,7 +162,6 @@ void xen_load_linux(PCMachineState *pcms);
>> void pc_memory_init(PCMachineState *pcms,
>>                     MemoryRegion *system_memory,
>>                     MemoryRegion *rom_memory,
>> -                    MemoryRegion **ram_memory,
>>                     uint64_t pci_hole64_size);
>> uint64_t pc_pci_hole64_start(void);
>> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 6e592bd969..8898cc9961 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -936,7 +936,6 @@ static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
>> void pc_memory_init(PCMachineState *pcms,
>>                     MemoryRegion *system_memory,
>>                     MemoryRegion *rom_memory,
>> -                    MemoryRegion **ram_memory,
>>                     uint64_t pci_hole64_size)
>> {
>>     int linux_boot, i;
>> @@ -994,7 +993,6 @@ void pc_memory_init(PCMachineState *pcms,
>>      * Split single memory region and use aliases to address portions of it,
>>      * done for backwards compatibility with older qemus.
>>      */
>> -    *ram_memory = machine->ram;
>>     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
>>     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", machine->ram,
>>                              0, x86ms->below_4g_mem_size);
>> 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);
>>     } else {
>> +        ram_memory = machine->ram;
>
>Maybe you could just replace the few places it's used with machine->ram directly and get rid of the local variable. There seems to be no advantage storing it in a local just to use it once (in q35 below) or twice in pc-piix. The local name is not even that much shorter so I don't see a reason to have it in the first place,

Possible with q35 but not with piix which needs to get the RAM from Xen if running in this mode. I'll adjust q35 then.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>>         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);
>> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>>     } else {
>>         pc_system_flash_cleanup_unused(pcms);
>>         if (machine->kernel_filename != NULL) {
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 8253b49296..88f0981f50 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -129,7 +129,7 @@ static void pc_q35_init(MachineState *machine)
>>     MemoryRegion *system_io = get_system_io();
>>     MemoryRegion *pci_memory;
>>     MemoryRegion *rom_memory;
>> -    MemoryRegion *ram_memory;
>> +    MemoryRegion *ram_memory = machine->ram;
>>     GSIState *gsi_state;
>>     ISABus *isa_bus;
>>     int i;
>> @@ -216,8 +216,7 @@ static void pc_q35_init(MachineState *machine)
>>     }
>> 
>>     /* allocate ram and load rom/bios */
>> -    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
>> -                   pci_hole64_size);
>> +    pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
>> 
>>     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
>>     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
>>
diff mbox series

Patch

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 66e3d059ef..b60b95921b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -162,7 +162,6 @@  void xen_load_linux(PCMachineState *pcms);
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory,
                     uint64_t pci_hole64_size);
 uint64_t pc_pci_hole64_start(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6e592bd969..8898cc9961 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -936,7 +936,6 @@  static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory,
                     uint64_t pci_hole64_size)
 {
     int linux_boot, i;
@@ -994,7 +993,6 @@  void pc_memory_init(PCMachineState *pcms,
      * Split single memory region and use aliases to address portions of it,
      * done for backwards compatibility with older qemus.
      */
-    *ram_memory = machine->ram;
     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", machine->ram,
                              0, x86ms->below_4g_mem_size);
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);
     } else {
+        ram_memory = machine->ram;
         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);
+        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
     } else {
         pc_system_flash_cleanup_unused(pcms);
         if (machine->kernel_filename != NULL) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8253b49296..88f0981f50 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -129,7 +129,7 @@  static void pc_q35_init(MachineState *machine)
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    MemoryRegion *ram_memory;
+    MemoryRegion *ram_memory = machine->ram;
     GSIState *gsi_state;
     ISABus *isa_bus;
     int i;
@@ -216,8 +216,7 @@  static void pc_q35_init(MachineState *machine)
     }
 
     /* allocate ram and load rom/bios */
-    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
-                   pci_hole64_size);
+    pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
 
     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,