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

[Mark Cave-Ayland]

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 <balaton@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.

>           ppc_boot_device = '\0';
>           /* We consider that NewWorld PowerMac never have any floppy drive
>            * For now, OHW cannot boot from the network.
> @@ -343,13 +332,6 @@ static void ppc_core99_init(MachineState *machine)
>       has_adb = (core99_machine->via_config == CORE99_VIA_CONFIG_CUDA ||
>                  core99_machine->via_config == CORE99_VIA_CONFIG_PMU_ADB);
>   
> -    /* Timebase Frequency */
> -    if (kvm_enabled()) {
> -        tbfreq = kvmppc_get_tbfreq();
> -    } else {
> -        tbfreq = TBFREQ;
> -    }
> -
>       /* init basic PC hardware */
>       pci_bus = PCI_HOST_BRIDGE(uninorth_pci)->bus;
>   
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 86512d31ad..cb67e44081 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -84,11 +84,11 @@ static void ppc_heathrow_init(MachineState *machine)
>       PowerPCCPU *cpu = NULL;
>       CPUPPCState *env = NULL;
>       char *filename;
> -    int i, bios_size;
> +    int i, bios_size = -1;
>       MemoryRegion *bios = g_new(MemoryRegion, 1);
>       uint64_t bios_addr;
> -    uint32_t kernel_base, initrd_base, cmdline_base = 0;
> -    int32_t kernel_size, initrd_size;
> +    uint32_t kernel_base = 0, initrd_base = 0, cmdline_base = 0;
> +    int32_t kernel_size = 0, initrd_size = 0;
>       PCIBus *pci_bus;
>       PCIDevice *macio;
>       MACIOIDEState *macio_ide;
> @@ -99,7 +99,7 @@ static void ppc_heathrow_init(MachineState *machine)
>       uint16_t ppc_boot_device;
>       DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>       void *fw_cfg;
> -    uint64_t tbfreq;
> +    uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
>   
>       /* init CPUs */
>       for (i = 0; i < machine->smp.cpus; i++) {
> @@ -139,8 +139,6 @@ static void ppc_heathrow_init(MachineState *machine)
>               bios_addr = PROM_BASE;
>           }
>           g_free(filename);
> -    } else {
> -        bios_size = -1;
>       }
>       if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) {
>           error_report("could not load PowerPC bios '%s'", bios_name);
> @@ -148,12 +146,10 @@ static void ppc_heathrow_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,
> @@ -186,16 +182,10 @@ static void ppc_heathrow_init(MachineState *machine)
>               }
>               cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>           } else {
> -            initrd_base = 0;
> -            initrd_size = 0;
>               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;
>           ppc_boot_device = '\0';
>           for (i = 0; machine->boot_config.order[i] != '\0'; i++) {
>               /*
> @@ -223,13 +213,6 @@ static void ppc_heathrow_init(MachineState *machine)
>           }
>       }
>   
> -    /* Timebase Frequency */
> -    if (kvm_enabled()) {
> -        tbfreq = kvmppc_get_tbfreq();
> -    } else {
> -        tbfreq = TBFREQ;
> -    }
> -
>       /* Grackle PCI host bridge */
>       grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>       qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);

... and obviously the same comments for mac_oldworld.c too.


ATB,

Mark.