Re: [Qemu-ppc] [PATCHv2 08/12] pseries: Consolidate construction of /rtas device tree node

[Michael Roth]

Quoting David Gibson (2016-10-20 21:56:36)
> For historical reasons construction of the /rtas node in the device
> tree (amongst others) is split into several places.  In particular
> it's split between spapr_create_fdt_skel(), spapr_build_fdt() and
> spapr_rtas_device_tree_setup().
> 
> In fact, as well as adding the actual RTAS tokens to the device tree,
> spapr_rtas_device_tree_setup() just adds the ibm,lrdr-capacity
> property, which despite going in the /rtas node, doesn't have a lot to
> do with RTAS.
> 
> This patch consolidates the code constructing /rtas together into a new
> spapr_dt_rtas() function.  spapr_rtas_device_tree_setup() is renamed to
> spapr_dt_rtas_tokens() and now only adds the token properties.
> 
> Signed-off-by: David Gibson <address@hidden>
> Reviewed-by: Thomas Huth <address@hidden>

Reviewed-by: Michael Roth <address@hidden>

> ---
>  hw/ppc/spapr.c         | 130 
> +++++++++++++++++++++++++++----------------------
>  hw/ppc/spapr_rtas.c    |  33 ++-----------
>  include/hw/ppc/spapr.h |   3 +-
>  3 files changed, 76 insertions(+), 90 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5a5d7b5..d69b234 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -274,25 +274,8 @@ static void add_str(GString *s, const gchar *s1)
>  static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
>  {
>      void *fdt;
> -    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)};
>      char *buf;
> 
> -    add_str(hypertas, "hcall-pft");
> -    add_str(hypertas, "hcall-term");
> -    add_str(hypertas, "hcall-dabr");
> -    add_str(hypertas, "hcall-interrupt");
> -    add_str(hypertas, "hcall-tce");
> -    add_str(hypertas, "hcall-vio");
> -    add_str(hypertas, "hcall-splpar");
> -    add_str(hypertas, "hcall-bulk");
> -    add_str(hypertas, "hcall-set-mode");
> -    add_str(hypertas, "hcall-sprg0");
> -    add_str(hypertas, "hcall-copy");
> -    add_str(hypertas, "hcall-debug");
> -    add_str(qemu_hypertas, "hcall-memop1");
> -
>      fdt = g_malloc0(FDT_MAX_SIZE);
>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> 
> @@ -333,41 +316,6 @@ static void *spapr_create_fdt_skel(sPAPRMachineState 
> *spapr)
>      _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
>      _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
> 
> -    /* RTAS */
> -    _FDT((fdt_begin_node(fdt, "rtas")));
> -
> -    if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> -        add_str(hypertas, "hcall-multi-tce");
> -    }
> -    _FDT((fdt_property(fdt, "ibm,hypertas-functions", hypertas->str,
> -                       hypertas->len)));
> -    g_string_free(hypertas, TRUE);
> -    _FDT((fdt_property(fdt, "qemu,hypertas-functions", qemu_hypertas->str,
> -                       qemu_hypertas->len)));
> -    g_string_free(qemu_hypertas, TRUE);
> -
> -    _FDT((fdt_property(fdt, "ibm,associativity-reference-points",
> -        refpoints, sizeof(refpoints))));
> -
> -    _FDT((fdt_property_cell(fdt, "rtas-error-log-max", RTAS_ERROR_LOG_MAX)));
> -    _FDT((fdt_property_cell(fdt, "rtas-event-scan-rate",
> -                            RTAS_EVENT_SCAN_RATE)));
> -
> -    if (msi_nonbroken) {
> -        _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0)));
> -    }
> -
> -    /*
> -     * According to PAPR, rtas ibm,os-term does not guarantee a return
> -     * back to the guest cpu.
> -     *
> -     * While an additional ibm,extended-os-term property indicates that
> -     * rtas call return will always occur. Set this property.
> -     */
> -    _FDT((fdt_property(fdt, "ibm,extended-os-term", NULL, 0)));
> -
> -    _FDT((fdt_end_node(fdt)));
> -
>      /* vdevice */
>      _FDT((fdt_begin_node(fdt, "vdevice")));
> 
> @@ -840,6 +788,75 @@ int spapr_h_cas_compose_response(sPAPRMachineState 
> *spapr,
>      return 0;
>  }
> 
> +static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
> +{
> +    int rtas;
> +    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) };
> +    uint64_t max_hotplug_addr = spapr->hotplug_memory.base +
> +        memory_region_size(&spapr->hotplug_memory.mr);
> +    uint32_t lrdr_capacity[] = {
> +        cpu_to_be32(max_hotplug_addr >> 32),
> +        cpu_to_be32(max_hotplug_addr & 0xffffffff),
> +        0, cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE),
> +        cpu_to_be32(max_cpus / smp_threads),
> +    };
> +
> +    _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
> +
> +    /* hypertas */
> +    add_str(hypertas, "hcall-pft");
> +    add_str(hypertas, "hcall-term");
> +    add_str(hypertas, "hcall-dabr");
> +    add_str(hypertas, "hcall-interrupt");
> +    add_str(hypertas, "hcall-tce");
> +    add_str(hypertas, "hcall-vio");
> +    add_str(hypertas, "hcall-splpar");
> +    add_str(hypertas, "hcall-bulk");
> +    add_str(hypertas, "hcall-set-mode");
> +    add_str(hypertas, "hcall-sprg0");
> +    add_str(hypertas, "hcall-copy");
> +    add_str(hypertas, "hcall-debug");
> +    add_str(qemu_hypertas, "hcall-memop1");
> +
> +    if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> +        add_str(hypertas, "hcall-multi-tce");
> +    }
> +    _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
> +                     hypertas->str, hypertas->len));
> +    g_string_free(hypertas, TRUE);
> +    _FDT(fdt_setprop(fdt, rtas, "qemu,hypertas-functions",
> +                     qemu_hypertas->str, qemu_hypertas->len));
> +    g_string_free(qemu_hypertas, TRUE);
> +
> +    _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
> +                     refpoints, sizeof(refpoints)));
> +
> +    _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
> +                          RTAS_ERROR_LOG_MAX));
> +    _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",
> +                          RTAS_EVENT_SCAN_RATE));
> +
> +    if (msi_nonbroken) {
> +        _FDT(fdt_setprop(fdt, rtas, "ibm,change-msix-capable", NULL, 0));
> +    }
> +
> +    /*
> +     * According to PAPR, rtas ibm,os-term does not guarantee a return
> +     * back to the guest cpu.
> +     *
> +     * While an additional ibm,extended-os-term property indicates
> +     * that rtas call return will always occur. Set this property.
> +     */
> +    _FDT(fdt_setprop(fdt, rtas, "ibm,extended-os-term", NULL, 0));
> +
> +    _FDT(fdt_setprop(fdt, rtas, "ibm,lrdr-capacity",
> +                     lrdr_capacity, sizeof(lrdr_capacity)));
> +
> +    spapr_dt_rtas_tokens(fdt, rtas);
> +}
> +
>  static void spapr_dt_chosen(sPAPRMachineState *spapr, void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
> @@ -949,12 +966,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
>          }
>      }
> 
> -    /* RTAS */
> -    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
> -    if (ret < 0) {
> -        error_report("Couldn't set up RTAS device tree properties");
> -    }
> -
>      /* cpus */
>      spapr_populate_cpus_dt_node(fdt, spapr);
> 
> @@ -972,6 +983,9 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
>          }
>      }
> 
> +    /* /rtas */
> +    spapr_dt_rtas(spapr, fdt);
> +
>      /* /chosen */
>      spapr_dt_chosen(spapr, fdt);
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 54b4ad3..bb19944 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -46,6 +46,7 @@
>  #include "hw/ppc/spapr_drc.h"
>  #include "qemu/cutils.h"
>  #include "trace.h"
> +#include "hw/ppc/fdt.h"
> 
>  static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
>                                                      uint32_t drc_index)
> @@ -710,16 +711,9 @@ void spapr_rtas_register(int token, const char *name, 
> spapr_rtas_fn fn)
>      rtas_table[token].fn = fn;
>  }
> 
> -int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
> -                                 hwaddr rtas_size)
> +void spapr_dt_rtas_tokens(void *fdt, int rtas)
>  {
> -    int ret;
>      int i;
> -    uint32_t lrdr_capacity[5];
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> -    uint64_t max_hotplug_addr = spapr->hotplug_memory.base +
> -                                
> memory_region_size(&spapr->hotplug_memory.mr);
> 
>      for (i = 0; i < RTAS_TOKEN_MAX - RTAS_TOKEN_BASE; i++) {
>          struct rtas_call *call = &rtas_table[i];
> @@ -728,29 +722,8 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr 
> rtas_addr,
>              continue;
>          }
> 
> -        ret = qemu_fdt_setprop_cell(fdt, "/rtas", call->name,
> -                                    i + RTAS_TOKEN_BASE);
> -        if (ret < 0) {
> -            error_report("Couldn't add rtas token for %s: %s",
> -                    call->name, fdt_strerror(ret));
> -            return ret;
> -        }
> -
> +        _FDT(fdt_setprop_cell(fdt, rtas, call->name, i + RTAS_TOKEN_BASE));
>      }
> -
> -    lrdr_capacity[0] = cpu_to_be32(max_hotplug_addr >> 32);
> -    lrdr_capacity[1] = cpu_to_be32(max_hotplug_addr & 0xffffffff);
> -    lrdr_capacity[2] = 0;
> -    lrdr_capacity[3] = cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE);
> -    lrdr_capacity[4] = cpu_to_be32(max_cpus/smp_threads);
> -    ret = qemu_fdt_setprop(fdt, "/rtas", "ibm,lrdr-capacity", lrdr_capacity,
> -                     sizeof(lrdr_capacity));
> -    if (ret < 0) {
> -        error_report("Couldn't add ibm,lrdr-capacity rtas property");
> -        return ret;
> -    }
> -
> -    return 0;
>  }
> 
>  void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 847ae88..8b8bb97 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -530,8 +530,7 @@ void spapr_rtas_register(int token, const char *name, 
> spapr_rtas_fn fn);
>  target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *sm,
>                               uint32_t token, uint32_t nargs, target_ulong 
> args,
>                               uint32_t nret, target_ulong rets);
> -int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
> -                                 hwaddr rtas_size);
> +void spapr_dt_rtas_tokens(void *fdt, int rtas);
>  void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr);
> 
>  #define SPAPR_TCE_PAGE_SHIFT   12
> -- 
> 2.7.4
>