| Message ID | 154774536496.1208625.12823909967079119637.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Add support for PHB hotplug | expand |
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 <groug@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. > 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; >
On Fri, 18 Jan 2019 14:38:31 +0100 Cédric Le Goater <clg@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 <groug@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. > > 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; > > >
On 1/22/19 2:27 PM, Greg Kurz wrote: > On Fri, 18 Jan 2019 14:38:31 +0100 > Cédric Le Goater <clg@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 <groug@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; >>> >> >
On Tue, 22 Jan 2019 15:26:45 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 1/22/19 2:27 PM, Greg Kurz wrote: > > On Fri, 18 Jan 2019 14:38:31 +0100 > > Cédric Le Goater <clg@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 <groug@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 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("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; > >>> > >> > > >
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)); + 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)); _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; + /* Routing table */ XiveEAS *eat; uint32_t nr_irqs;
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. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/intc/spapr_xive.c | 9 ++++----- include/hw/ppc/spapr_xive.h | 3 +++ 2 files changed, 7 insertions(+), 5 deletions(-)