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