Re: [Qemu-devel] [PATCH 04/13] pseries: Make spapr_create_fdt_skel() get information from machine state

From: Thomas Huth <thuth@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>,
	aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, groug@kaod.org
Cc: agraf@suse.de, lvivier@redhat.com, qemu-ppc@nongnu.org,
	qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 04/13] pseries: Make spapr_create_fdt_skel() get information from machine state
Date: Thu, 20 Oct 2016 09:19:25 +0200	[thread overview]
Message-ID: <6511c2db-180b-0720-2ebf-8b6d9e3dfe0e@redhat.com> (raw)
In-Reply-To: <1476940330-27705-5-git-send-email-david@gibson.dropbear.id.au>

On 20.10.2016 07:12, David Gibson wrote:
> Currently spapr_create_fdt_skel() takes a bunch of individual parameters
> for various things it will put in the device tree.  Some of these can
> already be taken directly from sPAPRMachineState.  This patch alters it so
> that all of them can be taken from there, which will allow this code to
> be moved away from its current caller in future.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/ppc/spapr.c         | 81 ++++++++++++++++++++++----------------------------
>  include/hw/ppc/spapr.h |  4 +++
>  2 files changed, 40 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c4baca9..25c16d3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -271,16 +271,12 @@ static void add_str(GString *s, const gchar *s1)
>      g_string_append_len(s, s1, strlen(s1) + 1);
>  }
>  
> -static void *spapr_create_fdt_skel(hwaddr initrd_base,
> -                                   hwaddr initrd_size,
> -                                   hwaddr kernel_size,
> -                                   bool little_endian,
> -                                   const char *kernel_cmdline,
> -                                   uint32_t epow_irq)
> +static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
>  {
> +    MachineState *machine = MACHINE(spapr);
>      void *fdt;
> -    uint32_t start_prop = cpu_to_be32(initrd_base);
> -    uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
> +    uint32_t start_prop = cpu_to_be32(spapr->initrd_base);
> +    uint32_t end_prop = cpu_to_be32(spapr->initrd_base + spapr->initrd_size);
>      GString *hypertas = g_string_sized_new(256);
>      GString *qemu_hypertas = g_string_sized_new(256);
>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> @@ -305,11 +301,13 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      fdt = g_malloc0(FDT_MAX_SIZE);
>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
>  
> -    if (kernel_size) {
> -        _FDT((fdt_add_reservemap_entry(fdt, KERNEL_LOAD_ADDR, kernel_size)));
> +    if (spapr->kernel_size) {
> +        _FDT((fdt_add_reservemap_entry(fdt, KERNEL_LOAD_ADDR,
> +                                       spapr->kernel_size)));
>      }
> -    if (initrd_size) {
> -        _FDT((fdt_add_reservemap_entry(fdt, initrd_base, initrd_size)));
> +    if (spapr->initrd_size) {
> +        _FDT((fdt_add_reservemap_entry(fdt, spapr->initrd_base,
> +                                       spapr->initrd_size)));
>      }
>      _FDT((fdt_finish_reservemap(fdt)));
>  
> @@ -354,17 +352,17 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      /* Set Form1_affinity */
>      _FDT((fdt_property(fdt, "ibm,architecture-vec-5", vec5, sizeof(vec5))));
>  
> -    _FDT((fdt_property_string(fdt, "bootargs", kernel_cmdline)));
> +    _FDT((fdt_property_string(fdt, "bootargs", machine->kernel_cmdline)));
>      _FDT((fdt_property(fdt, "linux,initrd-start",
>                         &start_prop, sizeof(start_prop))));
>      _FDT((fdt_property(fdt, "linux,initrd-end",
>                         &end_prop, sizeof(end_prop))));
> -    if (kernel_size) {
> +    if (spapr->kernel_size) {
>          uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
> -                              cpu_to_be64(kernel_size) };
> +                              cpu_to_be64(spapr->kernel_size) };
>  
>          _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
> -        if (little_endian) {
> +        if (spapr->kernel_le) {
>              _FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0)));
>          }
>      }
> @@ -441,7 +439,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      _FDT((fdt_end_node(fdt)));
>  
>      /* event-sources */
> -    spapr_events_fdt_skel(fdt, epow_irq);
> +    spapr_events_fdt_skel(fdt, spapr->check_exception_irq);
>  
>      /* /hypervisor node */
>      if (kvm_enabled()) {

Since kernel_size is used multiple times in this function, you could
simplify your patch a little bit by introducing a new local variable
"long kernel_size = spapr->kernel_size;" at the beginning of the
function... but anyway, patch looks good to me, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>