Re: [Qemu-devel] [PATCH v4 5/6] ppc: Don't duplicate QEMUMachineInitArgs in PPCE500Params

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 08/16/13 13:13, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Pass on the generic arguments unadulterated, and the machine-specific
> ones as separate argument.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ppc/e500.c      | 35 ++++++++++++++++++-----------------
>  hw/ppc/e500.h      | 13 +++----------
>  hw/ppc/e500plat.c  |  8 +-------
>  hw/ppc/mpc8544ds.c |  8 +-------
>  4 files changed, 23 insertions(+), 41 deletions(-)

Please always use

  -O/path/to/order_file

when invoking git-format-patch.

The contents of "order_file" should be minimally

  configure
  Makefile*
  *.json
  *.h
  *.c

It's much easier to review a patch when "declarative changes" are shown
first (ie. in approximate logical dependency order).

Then,

> -void ppce500_init(PPCE500Params *params)
> +void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
>  {
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> @@ -584,8 +585,8 @@ void ppce500_init(PPCE500Params *params)
>      PPCE500CCSRState *ccsr;
>  
>      /* Setup CPUs */
> -    if (params->cpu_model == NULL) {
> -        params->cpu_model = "e500v2_v30";
> +    if (args->cpu_model == NULL) {
> +        args->cpu_model = "e500v2_v30";
>      }

As discussed before, this change will modify the "args.cpu_model" member
in main(), but that's OK.


> @@ -634,7 +635,7 @@ void ppce500_init(PPCE500Params *params)
>  
>      /* Fixup Memory size on a alignment boundary */
>      ram_size &= ~(RAM_SIZES_ALIGN - 1);
> -    params->ram_size = ram_size;
> +    args->ram_size = ram_size;

This hackery (commendably left intact by the patch) is convincing me
that QEMUMachineInitArgs should not have a "ram_size" member at all. If
"ram_size" is a well-founded global, then let's treat it as such. Whatever.

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