Re: [Qemu-devel] [PATCH v4 3/6] sun4: Don't prematurely explode QEMUMachineInitArgs

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 08/16/13 13:13, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Don't explode QEMUMachineInitArgs before passing it to
> sun4m_hw_init(), sun4uv_init().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/sparc/sun4m.c   | 113 ++++++++++++-----------------------------------------
>  hw/sparc64/sun4u.c |  52 +++++++-----------------
>  2 files changed, 40 insertions(+), 125 deletions(-)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 942ca37..36ef36f 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -836,12 +836,10 @@ static void dummy_fdc_tc(void *opaque, int irq, int level)
>  {
>  }
>  
> -static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
> -                          const char *boot_device,
> -                          const char *kernel_filename,
> -                          const char *kernel_cmdline,
> -                          const char *initrd_filename, const char *cpu_model)
> +static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
> +                          QEMUMachineInitArgs *args)
>  {
> +    const char *cpu_model = args->cpu_model;

This may not be strictly necessary; in the first patch you modify
args->cpu_model too.

In any case the above is not wrong.

> @@ -878,13 +875,15 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>  
>      initrd_size = 0;
>      initrd_addr = 0;
> -    kernel_size = sun4u_load_kernel(kernel_filename, initrd_filename,
> +    kernel_size = sun4u_load_kernel(args->kernel_filename,
> +                                    args->initrd_filename,
>                                      ram_size, &initrd_size, &initrd_addr,
>                                      &kernel_addr, &kernel_entry);

The patch is correct / faithful here, but I smell some fubar in the code
that the patch keeps intact.

"ram_size" is apparently the global variable from "vl.c". So this
function gets the RAM size twice, once via RAM_size / args->ram_size,
then via the "ram_size" global. (They should have identical values,
"args.ram_size" in main() is initialized with "ram_size" the global.)

The rest of the code below this hunk (in the full source file, not just
in the patch) alternates between RAM_size / args->ram_size and
"ram_size" quite schizophrenically too; see eg. FW_CFG_RAM_SIZE.

Anyway the patch only improves things.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>