Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables

[Mark Cave-Ayland]

On 03/10/2022 23:08, BALATON Zoltan wrote:

On Thu, 29 Sep 2022, Mark Cave-Ayland wrote:

On 25/09/2022 10:16, BALATON Zoltan wrote:

On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:

On 17/09/2022 00:07, BALATON Zoltan wrote:

Some lines can be dropped making the code flow simpler and easier to
follow by setting default values at variable declaration for some
variables in both mac_oldworld.c and mac_newworld.c.

Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
  hw/ppc/mac_newworld.c | 28 +++++-----------------------
  hw/ppc/mac_oldworld.c | 27 +++++----------------------
  2 files changed, 10 insertions(+), 45 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 27e4e8d136..6bc3bd19be 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -111,11 +111,11 @@ static void ppc_core99_init(MachineState *machine)
      CPUPPCState *env = NULL;
      char *filename;
      IrqLines *openpic_irqs;
-    int i, j, k, ppc_boot_device, machine_arch, bios_size;
+    int i, j, k, ppc_boot_device, machine_arch, bios_size = -1;
      const char *bios_name = machine->firmware ?: PROM_FILENAME;
      MemoryRegion *bios = g_new(MemoryRegion, 1);
-    hwaddr kernel_base, initrd_base, cmdline_base = 0;
-    long kernel_size, initrd_size;
+    hwaddr kernel_base = 0, initrd_base = 0, cmdline_base = 0;
+    long kernel_size = 0, initrd_size = 0;
      UNINHostState *uninorth_pci;
      PCIBus *pci_bus;
      PCIDevice *macio;
@@ -130,7 +130,7 @@ static void ppc_core99_init(MachineState *machine)
      DeviceState *dev, *pic_dev;
      DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
      hwaddr nvram_addr = 0xFFF04000;
-    uint64_t tbfreq;
+    uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
        /* init CPUs */
      for (i = 0; i < machine->smp.cpus; i++) {
@@ -165,8 +165,6 @@ static void ppc_core99_init(MachineState *machine)
              bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
          }
          g_free(filename);
-    } else {
-        bios_size = -1;
      }
      if (bios_size < 0 || bios_size > PROM_SIZE) {
          error_report("could not load PowerPC bios '%s'", bios_name);
@@ -174,15 +172,12 @@ static void ppc_core99_init(MachineState *machine)
      }
        if (machine->kernel_filename) {
-        int bswap_needed;
+        int bswap_needed = 0;
    #ifdef BSWAP_NEEDED
          bswap_needed = 1;
-#else
-        bswap_needed = 0;
  #endif
          kernel_base = KERNEL_LOAD_ADDR;
-
          kernel_size = load_elf(machine->kernel_filename, NULL,
                                 translate_kernel_address, NULL, NULL, NULL,
                                 NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
@@ -212,16 +207,10 @@ static void ppc_core99_init(MachineState *machine)
              }
              cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
          } else {
-            initrd_base = 0;
-            initrd_size = 0;

This bit seems a bit odd...

              cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);

          }
          ppc_boot_device = 'm';
      } else {
-        kernel_base = 0;
-        kernel_size = 0;
-        initrd_base = 0;
-        initrd_size = 0;

and also here. The only reason I can think that someone would explicitly set these variables back to 0 would be if there are cases in the load_*() functions where non-zero values could be returned for failure. It's worth having a look to confirm this and see if this also needs some additional tweaks to the logic flow here.

They aren't set back to 0 but set here the first time. Nothing touches these variables before this if-else do this patch just moves the zero init to the variable declaration and only leaves the cases which set a value different than zero here which I think is easier to follow.

Okay - in that case if you can test with a non-kernel ELF to verify this, and add a note confirming that everything still works for the error paths then that will be fine.

I've originally added non-elf loading to be able to use -bios macrom.bin which I've now verified that it still works so this should be OK. I've also split this patch up to more parts for easier review in the later versions of the series but what it does is basically instead of

int x;
if (cond) {
   x = a;
} else {
   x = 0;
}

we do

int x = 0;
if (cond) {
   x = a;
}

which I thought would be simple to review.

Great - thanks for confirming that this still works for all intended cases.


ATB,

Mark.