On 1/22/19 2:27 PM, Greg Kurz wrote:
> On Fri, 18 Jan 2019 14:38:31 +0100
> Cédric Le Goater <c...@kaod.org> 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 <gr...@kaod.org>
>>> ---
>>> 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("interrupt-controller@%" 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("interrupt-controller@%" 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;
>>>
>>
>
