Re: [qemu-s390x] [PATCH v3 15/19] spapr_xive: Cache device tree nodename in sPAPRXive

[Cédric Le Goater]

On 1/17/19 6:16 PM, Greg Kurz wrote:
> PHB hotplug will need to know the name of the XIVE controller node. Since
> it is an invariant for the machine lifetime, compute it at realize and
> store it under the sPAPRXive structure.
Could you please gather all these changes [15-17] in one patch. It would 
be easier to track. 

> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  hw/intc/spapr_xive.c        |    9 ++++-----
>  include/hw/ppc/spapr_xive.h |    3 +++
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index e542ae59d7fd..9732c3d89c77 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -316,6 +316,9 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>      /* Map all regions */
>      spapr_xive_map_mmio(xive);
>  
> +    xive->nodename = g_strdup_printf("address@hidden" PRIx64,
> +                           xive->tm_base + XIVE_TM_USER_PAGE * (1 << 
> TM_SHIFT));

I would use a helper routine instead.
 
>      qemu_register_reset(spapr_xive_reset, dev);
>  }
>  
> @@ -1436,7 +1439,6 @@ int spapr_dt_xive(sPAPRMachineState *spapr, uint32_t 
> nr_servers, void *fdt)
>          cpu_to_be32(7),    /* start */
>          cpu_to_be32(0xf8), /* count */
>      };
> -    gchar *nodename;
>  
>      /*
>       * The "ibm,plat-res-int-priorities" property defines the priority
> @@ -1453,10 +1455,7 @@ int spapr_dt_xive(sPAPRMachineState *spapr, uint32_t 
> nr_servers, void *fdt)
>                             XIVE_TM_OS_PAGE * (1ull << TM_SHIFT));
>      timas[3] = cpu_to_be64(1ull << TM_SHIFT);
>  
> -    nodename = g_strdup_printf("address@hidden" PRIx64,
> -                           xive->tm_base + XIVE_TM_USER_PAGE * (1 << 
> TM_SHIFT));
> -    _FDT(node = fdt_add_subnode(fdt, 0, nodename));
> -    g_free(nodename);
> +    _FDT(node = fdt_add_subnode(fdt, 0, xive->nodename));

and use the helper routine here.

>      _FDT(fdt_setprop_string(fdt, node, "device_type", "power-ivpe"));
>      _FDT(fdt_setprop(fdt, node, "reg", timas, sizeof(timas)));
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 29dafead9ba0..deea34b03ee5 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -26,6 +26,9 @@ typedef struct sPAPRXive {
>      XiveENDSource end_source;
>      hwaddr        end_base;
>  
> +    /* DT */
> +    gchar *nodename;

I don't think this is needed. See other patches for why.

Thanks,

C.
 
>
>      /* Routing table */
>      XiveEAS       *eat;
>      uint32_t      nr_irqs;
>