Laszlo Ersek <ler...@redhat.com> writes:
> comments below
>
> On 08/16/13 13:13, arm...@redhat.com wrote:
>> From: Markus Armbruster <arm...@redhat.com>
>>
>> Don't explode QEMUMachineInitArgs before passing it to
>> sun4m_hw_init(), sun4uv_init().
>>
>> Signed-off-by: Markus Armbruster <arm...@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.
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 <ler...@redhat.com>
Thanks!
