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

[Greg Kurz]

On Tue, 22 Jan 2019 15:26:45 +0100
Cédric Le Goater <address@hidden> wrote:

> On 1/22/19 2:27 PM, Greg Kurz wrote:
> > On Fri, 18 Jan 2019 14:38:31 +0100
> > Cédric Le Goater <address@hidden> wrote:
> >   
> >> 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. 
> >>  
> > 
> > I'll do that in v4.
> >   
> >>> 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.
> >>    
> > 
> > Ok, I see your suggestion to put the name in a static. Well, it would work
> > as long as we only have one interrupt controller around. It is the case now
> > but could this change in the future ? If not then I'm perfectly fine with
> > your suggestions, otherwise I guess the nodename should be under sPAPRXive. 
> >  
> 
> Well, I am not sure it's worth extending SPAPRXive for a name that can 
> be computed. Using a static is probably not a better solution. 
> 
> Maybe return the node offset instead. See pnv_chip_isa_offset() in pnv.c 

We can't with pseries because SLOF can update the machine's fdt, which
means the offset could be different from what it was when spapr_dt_xive()
was last called.

> for a similar issue.
> 
> Thanks,
> 
> C. 
> 
> >   
> >>>      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;
> >>>     
> >>  
> >   
>