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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> comments below
>
> On 08/16/13 13:13, address@hidden wrote:
>> From: Markus Armbruster <address@hidden>
>> 
>> Don't explode QEMUMachineInitArgs before passing it to
>> sun4m_hw_init(), sun4uv_init().
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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.

Yes.

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

Correct.  Could be cleaned up on top.

> Anyway the patch only improves things.
>
> Reviewed-by: Laszlo Ersek <address@hidden>

Thanks!