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

[Mark Cave-Ayland]

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.