Re: [Qemu-devel] [PATCH qemu] spapr: Render full FDT on ibm, client-architecture-support

[David Gibson]

On Mon, Aug 26, 2019 at 02:31:26PM +1000, Alexey Kardashevskiy wrote:
> The ibm,client-architecture-support call is a way for the guest to
> negotiate capabilities with a hypervisor. It is implemented as:
> - the guest calls SLOF via client interface;
> - SLOF calls QEMU (H_CAS hypercall) with an options vector from the guest;
> - QEMU returns a device tree diff (which uses FDT format with
> an additional header before it);
> - SLOF walks through the partial diff tree and updates its internal tree
> with the values from the diff.
> 
> This changes QEMU to simply re-render the entire tree and send it as
> an update. SLOF can handle this already mostly, [1] is needed before this
> can be applied.
> 
> The benefit is reduced code size as there is no need for another set of
> DT rendering helpers such as spapr_fixup_cpu_dt().
> 
> The downside is that the updates are bigger now (as they include all
> nodes and properties) but the difference on a '-smp 256,threads=1' system
> before/after is 2.35s vs. 2.5s.
> 
> While at this, add a missing g_free(fdt) if the resulting tree is bigger
> than the space allocated by SLOF. Also, store the resulting tree in
> the spapr machine to have the latest valid FDT copy possible (this should
> not matter much as H_UPDATE_DT happens right after that but nevertheless).
> 
> [1] https://patchwork.ozlabs.org/patch/1152915/
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>

Reviewed-by: David Gibson <address@hidden>

Can you wrap that up with the SLOF update in a pull request for me?


> ---
>  hw/ppc/spapr.c | 90 ++++++--------------------------------------------
>  1 file changed, 10 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index baedadf20b8c..6dea5947afbc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -295,65 +295,6 @@ static void spapr_populate_pa_features(SpaprMachineState 
> *spapr,
>      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, 
> pa_size)));
>  }
>  
> -static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
> -{
> -    MachineState *ms = MACHINE(spapr);
> -    int ret = 0, offset, cpus_offset;
> -    CPUState *cs;
> -    char cpu_model[32];
> -    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> -
> -    CPU_FOREACH(cs) {
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        DeviceClass *dc = DEVICE_GET_CLASS(cs);
> -        int index = spapr_get_vcpu_id(cpu);
> -        int compat_smt = MIN(ms->smp.threads, ppc_compat_max_vthreads(cpu));
> -
> -        if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
> -            continue;
> -        }
> -
> -        snprintf(cpu_model, 32, "%s@%x", dc->fw_name, index);
> -
> -        cpus_offset = fdt_path_offset(fdt, "/cpus");
> -        if (cpus_offset < 0) {
> -            cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> -            if (cpus_offset < 0) {
> -                return cpus_offset;
> -            }
> -        }
> -        offset = fdt_subnode_offset(fdt, cpus_offset, cpu_model);
> -        if (offset < 0) {
> -            offset = fdt_add_subnode(fdt, cpus_offset, cpu_model);
> -            if (offset < 0) {
> -                return offset;
> -            }
> -        }
> -
> -        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> -                          pft_size_prop, sizeof(pft_size_prop));
> -        if (ret < 0) {
> -            return ret;
> -        }
> -
> -        if (nb_numa_nodes > 1) {
> -            ret = spapr_fixup_cpu_numa_dt(fdt, offset, cpu);
> -            if (ret < 0) {
> -                return ret;
> -            }
> -        }
> -
> -        ret = spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -
> -        spapr_populate_pa_features(spapr, cpu, fdt, offset,
> -                                   spapr->cas_legacy_guest_workaround);
> -    }
> -    return ret;
> -}
> -
>  static hwaddr spapr_node0_size(MachineState *machine)
>  {
>      if (nb_numa_nodes) {
> @@ -983,11 +924,13 @@ static bool spapr_hotplugged_dev_before_cas(void)
>      return false;
>  }
>  
> +static void *spapr_build_fdt(SpaprMachineState *spapr);
> +
>  int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>                                   target_ulong addr, target_ulong size,
>                                   SpaprOptionVector *ov5_updates)
>  {
> -    void *fdt, *fdt_skel;
> +    void *fdt;
>      SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
>  
>      if (spapr_hotplugged_dev_before_cas()) {
> @@ -1003,28 +946,11 @@ int spapr_h_cas_compose_response(SpaprMachineState 
> *spapr,
>  
>      size -= sizeof(hdr);
>  
> -    /* Create skeleton */
> -    fdt_skel = g_malloc0(size);
> -    _FDT((fdt_create(fdt_skel, size)));
> -    _FDT((fdt_finish_reservemap(fdt_skel)));
> -    _FDT((fdt_begin_node(fdt_skel, "")));
> -    _FDT((fdt_end_node(fdt_skel)));
> -    _FDT((fdt_finish(fdt_skel)));
> -    fdt = g_malloc0(size);
> -    _FDT((fdt_open_into(fdt_skel, fdt, size)));
> -    g_free(fdt_skel);
> -
> -    /* Fixup cpu nodes */
> -    _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
> -
> -    if (spapr_dt_cas_updates(spapr, fdt, ov5_updates)) {
> -        return -1;
> -    }
> -
> -    /* Pack resulting tree */
> +    fdt = spapr_build_fdt(spapr);
>      _FDT((fdt_pack(fdt)));
>  
>      if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> +        g_free(fdt);
>          trace_spapr_cas_failed(size);
>          return -1;
>      }
> @@ -1032,7 +958,11 @@ int spapr_h_cas_compose_response(SpaprMachineState 
> *spapr,
>      cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
>      cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
>      trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> -    g_free(fdt);
> +
> +    g_free(spapr->fdt_blob);
> +    spapr->fdt_size = fdt_totalsize(fdt);
> +    spapr->fdt_initial_size = spapr->fdt_size;
> +    spapr->fdt_blob = fdt;
>  
>      return 0;
>  }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature