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

[Cédric Le Goater]

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 
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;
>>>   
>>
>