diff mbox

[v4,5/6] ppc: Don't duplicate QEMUMachineInitArgs in PPCE500Params

Message ID 1376651630-9151-6-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Aug. 16, 2013, 11:13 a.m. UTC
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(-)

Comments

Laszlo Ersek Aug. 17, 2013, 10:46 a.m. UTC | #1
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>
Markus Armbruster Aug. 19, 2013, 9:24 a.m. UTC | #2
Laszlo Ersek <lersek@redhat.com> writes:

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

Is there a way to put this in .git/config?

Should http://wiki.qemu.org/Contribute/SubmitAPatch ask for this?

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

Global variables are often bad style (there are exceptions).  Even worse
is passing what is essentially global state down call chains while
keeping the global variables around for random poking.  And that's what
we tend to do %-/

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

Thanks!
Laszlo Ersek Aug. 19, 2013, 9:36 a.m. UTC | #3
On 08/19/13 11:24, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:

>> 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).
> 
> Is there a way to put this in .git/config?

The only way we've found thus far is to introduce a new alias for
git-format-patch that hardwires it.

> Should http://wiki.qemu.org/Contribute/SubmitAPatch ask for this?

Not a bad idea, but I'm afraid new contributors don't read it because
they don't know about it, and veteran contributors don't read it because
they don't need it.

The wiki would need a usable table of contents anyway, I have great
trouble every time I want to find anything. Wikipedia probably should
not have a flat sitemap for its millions of articles. The qemu wiki
should, for its handful.

Laszlo

> 
>> 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.
> 
> Global variables are often bad style (there are exceptions).  Even worse
> is passing what is essentially global state down call chains while
> keeping the global variables around for random poking.  And that's what
> we tend to do %-/
> 
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
>
diff mbox

Patch

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index f00a62a..e79612b 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -124,13 +124,14 @@  static void dt_serial_create(void *fdt, unsigned long long offset,
 }
 
 static int ppce500_load_device_tree(CPUPPCState *env,
+                                    QEMUMachineInitArgs *args,
                                     PPCE500Params *params,
                                     hwaddr addr,
                                     hwaddr initrd_base,
                                     hwaddr initrd_size)
 {
     int ret = -1;
-    uint64_t mem_reg_property[] = { 0, cpu_to_be64(params->ram_size) };
+    uint64_t mem_reg_property[] = { 0, cpu_to_be64(args->ram_size) };
     int fdt_size;
     void *fdt;
     uint8_t hypercall[16];
@@ -205,7 +206,7 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     }
 
     ret = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
-                                      params->kernel_cmdline);
+                                      args->kernel_cmdline);
     if (ret < 0)
         fprintf(stderr, "couldn't set /chosen/bootargs\n");
 
@@ -559,7 +560,7 @@  static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     return mpic;
 }
 
-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";
     }
 
     irqs = g_malloc0(smp_cpus * sizeof(qemu_irq *));
@@ -595,7 +596,7 @@  void ppce500_init(PPCE500Params *params)
         CPUState *cs;
         qemu_irq *input;
 
-        cpu = cpu_ppc_init(params->cpu_model);
+        cpu = cpu_ppc_init(args->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to initialize CPU!\n");
             exit(1);
@@ -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;
 
     /* Register Memory */
     memory_region_init_ram(ram, NULL, "mpc8544ds.ram", ram_size);
@@ -701,11 +702,11 @@  void ppce500_init(PPCE500Params *params)
     sysbus_create_simple("e500-spin", MPC8544_SPIN_BASE, NULL);
 
     /* Load kernel. */
-    if (params->kernel_filename) {
-        kernel_size = load_uimage(params->kernel_filename, &entry,
+    if (args->kernel_filename) {
+        kernel_size = load_uimage(args->kernel_filename, &entry,
                                   &loadaddr, NULL);
         if (kernel_size < 0) {
-            kernel_size = load_elf(params->kernel_filename, NULL, NULL,
+            kernel_size = load_elf(args->kernel_filename, NULL, NULL,
                                    &elf_entry, &elf_lowaddr, NULL, 1,
                                    ELF_MACHINE, 0);
             entry = elf_entry;
@@ -714,7 +715,7 @@  void ppce500_init(PPCE500Params *params)
         /* XXX try again as binary */
         if (kernel_size < 0) {
             fprintf(stderr, "qemu: could not load kernel '%s'\n",
-                    params->kernel_filename);
+                    args->kernel_filename);
             exit(1);
         }
 
@@ -726,14 +727,14 @@  void ppce500_init(PPCE500Params *params)
     }
 
     /* Load initrd. */
-    if (params->initrd_filename) {
+    if (args->initrd_filename) {
         initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-        initrd_size = load_image_targphys(params->initrd_filename, initrd_base,
+        initrd_size = load_image_targphys(args->initrd_filename, initrd_base,
                                           ram_size - initrd_base);
 
         if (initrd_size < 0) {
             fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-                    params->initrd_filename);
+                    args->initrd_filename);
             exit(1);
         }
 
@@ -741,12 +742,12 @@  void ppce500_init(PPCE500Params *params)
     }
 
     /* If we're loading a kernel directly, we must load the device tree too. */
-    if (params->kernel_filename) {
+    if (args->kernel_filename) {
         struct boot_info *boot_info;
         int dt_size;
 
-        dt_size = ppce500_load_device_tree(env, params, dt_base, initrd_base,
-                                           initrd_size);
+        dt_size = ppce500_load_device_tree(env, args, params, dt_base,
+                                           initrd_base, initrd_size);
         if (dt_size < 0) {
             fprintf(stderr, "couldn't load device tree\n");
             exit(1);
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 226c93d..52726a2 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -1,25 +1,18 @@ 
 #ifndef PPCE500_H
 #define PPCE500_H
 
+#include "hw/boards.h"
+
 typedef struct PPCE500Params {
-    /* Standard QEMU machine init params */
-    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;
     int pci_first_slot;
     int pci_nr_slots;
 
-    /* e500-specific params */
-
     /* required -- must at least add toplevel board compatible */
     void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
 
     int mpic_version;
 } PPCE500Params;
 
-void ppce500_init(PPCE500Params *params);
+void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params);
 
 #endif
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index a78de07..bf65b69 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -31,12 +31,6 @@  static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
 static void e500plat_init(QEMUMachineInitArgs *args)
 {
     PPCE500Params params = {
-        .ram_size = args->ram_size,
-        .boot_device = args->boot_device,
-        .kernel_filename = args->kernel_filename,
-        .kernel_cmdline = args->kernel_cmdline,
-        .initrd_filename = args->initrd_filename,
-        .cpu_model = args->cpu_model,
         .pci_first_slot = 0x1,
         .pci_nr_slots = PCI_SLOT_MAX - 1,
         .fixup_devtree = e500plat_fixup_devtree,
@@ -49,7 +43,7 @@  static void e500plat_init(QEMUMachineInitArgs *args)
         params.mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
     }
 
-    ppce500_init(&params);
+    ppce500_init(args, &params);
 }
 
 static QEMUMachine e500plat_machine = {
diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
index 4e551af..1888e75 100644
--- a/hw/ppc/mpc8544ds.c
+++ b/hw/ppc/mpc8544ds.c
@@ -29,19 +29,13 @@  static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
 static void mpc8544ds_init(QEMUMachineInitArgs *args)
 {
     PPCE500Params params = {
-        .ram_size = args->ram_size,
-        .boot_device = args->boot_device,
-        .kernel_filename = args->kernel_filename,
-        .kernel_cmdline = args->kernel_cmdline,
-        .initrd_filename = args->initrd_filename,
-        .cpu_model = args->cpu_model,
         .pci_first_slot = 0x11,
         .pci_nr_slots = 2,
         .fixup_devtree = mpc8544ds_fixup_devtree,
         .mpic_version = OPENPIC_MODEL_FSL_MPIC_20,
     };
 
-    ppce500_init(&params);
+    ppce500_init(args, &params);
 }