Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation

[Mark Cave-Ayland]

On 03/10/2022 21:13, BALATON Zoltan wrote:

> By slight reorganisation we can avoid an else branch and some code
> duplication which makes it easier to follow the code.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/mac_newworld.c | 6 +++---
>   hw/ppc/mac_oldworld.c | 7 +++----
>   2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 6bc3bd19be..73b01e8c6d 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
>                            machine->kernel_filename);
>               exit(1);
>           }
> +        cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> +                                         KERNEL_GAP);
>           /* load initrd */
>           if (machine->initrd_filename) {
> -            initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> KERNEL_GAP);
> +            initrd_base = cmdline_base;
>               initrd_size = load_image_targphys(machine->initrd_filename,
>                                                 initrd_base,
>                                                 machine->ram_size - 
> initrd_base);
> @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
>                   exit(1);
>               }
>               cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> -        } else {
> -            cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> KERNEL_GAP);
>           }
>           ppc_boot_device = 'm';
>       } else {
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index cb67e44081..b424729a39 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
>                            machine->kernel_filename);
>               exit(1);
>           }
> +        cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> +                                         KERNEL_GAP);
>           /* load initrd */
>           if (machine->initrd_filename) {
> -            initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> -                                            KERNEL_GAP);
> +            initrd_base = cmdline_base;
>               initrd_size = load_image_targphys(machine->initrd_filename,
>                                                 initrd_base,
>                                                 machine->ram_size - 
> initrd_base);
> @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
>                   exit(1);
>               }
>               cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> -        } else {
> -            cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> KERNEL_GAP);
>           }
>           ppc_boot_device = 'm';
>       } else {

Is there any particular reason why you would want to avoid the else branch? I don't 
feel this patch is an improvement since by always setting cmdline_base to a non-zero 
value, as a reviewer I then have to check all other uses of cmdline_base in the file 
to ensure that this doesn't cause any issues.

I much prefer the existing version since setting the values of cmdline_base and 
initrd_base is very clearly scoped within the if statement.


ATB,

Mark.