Re: [Qemu-devel] [RFC for-2.7 02/11] pseries: Split device tree construction from device tree load

[Thomas Huth <thuth@redhat.com>]

On 20.04.2016 04:33, David Gibson wrote:
> spapr_finalize_fdt() both finishes building the device tree for the guest
> and loads it into guest memory.  For future cleanups, it's going to be
> more convenient to do these two things separately, so split them apart.
> The loading portion is pretty trivial, so we move it inline into the
> caller, ppc_spapr_reset().
> 
> We also rename spapr_finalize_fdt(), because the current name is going to
> become inaccurate.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 41 ++++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index feaab08..26b95ce 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -899,10 +899,9 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
>      return 0;
>  }
>  
> -static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> -                               hwaddr fdt_addr,
> -                               hwaddr rtas_addr,
> -                               hwaddr rtas_size)
> +static void *spapr_build_fdt(sPAPRMachineState *spapr,
> +                             hwaddr rtas_addr,
> +                             hwaddr rtas_size)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> @@ -988,19 +987,8 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>          _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
>      }
>  
> -    _FDT((fdt_pack(fdt)));
> -
> -    if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> -        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> -                     fdt_totalsize(fdt), FDT_MAX_SIZE);
> -        exit(1);
> -    }
> -
> -    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> -    cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> -
>      g_free(bootlist);
> -    g_free(fdt);
> +    return fdt;
>  }
>  
>  static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
> @@ -1138,6 +1126,8 @@ static void ppc_spapr_reset(void)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>      PowerPCCPU *first_ppc_cpu;
>      uint32_t rtas_limit;
> +    void *fdt;
> +    int rc;
>  
>      /* Check for unknown sysbus devices */
>      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> @@ -1164,14 +1154,27 @@ static void ppc_spapr_reset(void)
>      spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE;
>      spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE;
>  
> -    /* Load the fdt */
> -    spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
> -                       spapr->rtas_size);
> +    fdt = spapr_build_fdt(spapr, spapr->rtas_addr, spapr->rtas_size);
>  
>      /* Copy RTAS over */
>      cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
>                                spapr->rtas_size);
>  
> +    rc = fdt_pack(fdt);
> +
> +    /* Should only fail if we've built a corrupted tree */
> +    assert(rc == 0);
> +
> +    if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> +        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> +                     fdt_totalsize(fdt), FDT_MAX_SIZE);
> +        exit(1);
> +    }
> +
> +    /* Load the fdt */
> +    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> +    cpu_physical_memory_write(spapr->fdt_addr, fdt, fdt_totalsize(fdt));
> +
>      /* Set up the entry state */
>      first_ppc_cpu = POWERPC_CPU(first_cpu);
>      first_ppc_cpu->env.gpr[3] = spapr->fdt_addr;

Looks good.

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