| Message ID | f6b04802d0a62668ba99c0086d0dda8ad103a65d.1663368422.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc ppc/mac machines clean up | expand |
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.
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 <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. 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. Regards, BALATON Zoltan >> 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. > >
On Sun, 25 Sep 2022, 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 <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. > > 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. Checked again in the original before this patch to make sure but these are in else branches of ifs setting the same variable to some value so I think it's either set to value or 0 and these lines settin 0 can'r run if the other part setting a value has run so these can't set it back, these are just the default 0 values so setting that at the beginning can drop these lines. What am I missing? How can these set a value back to 0? Regards, BALATON Zoltan
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 <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. > > 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. ATB, Mark.
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 <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. >> >> 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. Regards, BALATON Zoltan
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 <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. >>> >>> 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.
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; 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'; /* 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);
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(-)