The proper way to do that would be to use device_class_set_parent_realize(),
but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
is fine for now.
This should probably be achieved with a device realize hook in the
PSI base class and device_class_set_parent_realize() in the children
classes.
Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ppc/pnv_psi.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 6c94781e377d..546232106756 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
device_reset(DEVICE(dev));
}
+static void pnv_psi_realize(DeviceState *dev, Error **errp)
+{
+ PnvPsi *psi = PNV_PSI(dev);
+
+ /* Default BAR for MMIO region */
+ pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
+
+ qemu_register_reset(pnv_psi_reset_handler, dev);
+}
+
static void pnv_psi_power8_instance_init(Object *obj)
{
Pnv8Psi *psi8 = PNV8_PSI(obj);
@@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
"psihb", PNV_PSIHB_SIZE);
- /* Default BAR for MMIO region */
- pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
-
/* Default sources in XIVR */
for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
uint8_t xivr = irq_to_xivr[i];
@@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
((uint64_t) i << PSIHB_XIVR_SRC_SH);
}
- qemu_register_reset(pnv_psi_reset_handler, dev);
+ pnv_psi_realize(dev, errp);
}
static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
@@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
"psihb", PNV9_PSIHB_SIZE);
- pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
-
- qemu_register_reset(pnv_psi_reset_handler, dev);
+ pnv_psi_realize(dev, errp);
}
static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
Hi Greg,
On 1/7/20 5:32 PM, Greg Kurz wrote:
> The proper way to do that would be to use device_class_set_parent_realize(),
> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> is fine for now.
>
> This should probably be achieved with a device realize hook in the
> PSI base class and device_class_set_parent_realize() in the children
> classes.
Can you add a note explaining why the POWER10 PSI doesn't need it?
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/pnv_psi.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 6c94781e377d..546232106756 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> device_reset(DEVICE(dev));
> }
>
> +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> +{
> + PnvPsi *psi = PNV_PSI(dev);
> +
> + /* Default BAR for MMIO region */
> + pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> +
> + qemu_register_reset(pnv_psi_reset_handler, dev);
> +}
> +
> static void pnv_psi_power8_instance_init(Object *obj)
> {
> Pnv8Psi *psi8 = PNV8_PSI(obj);
> @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> "psihb", PNV_PSIHB_SIZE);
>
> - /* Default BAR for MMIO region */
> - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
> /* Default sources in XIVR */
> for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> uint8_t xivr = irq_to_xivr[i];
> @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> }
>
> - qemu_register_reset(pnv_psi_reset_handler, dev);
> + pnv_psi_realize(dev, errp);
> }
>
> static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> "psihb", PNV9_PSIHB_SIZE);
>
> - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
> - qemu_register_reset(pnv_psi_reset_handler, dev);
> + pnv_psi_realize(dev, errp);
> }
>
> static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>
>
On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Greg,
>
> On 1/7/20 5:32 PM, Greg Kurz wrote:
> > The proper way to do that would be to use device_class_set_parent_realize(),
> > but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> > pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> > is fine for now.
> >
> > This should probably be achieved with a device realize hook in the
> > PSI base class and device_class_set_parent_realize() in the children
> > classes.
>
> Can you add a note explaining why the POWER10 PSI doesn't need it?
For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
doesn't arise.
>
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> > hw/ppc/pnv_psi.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > index 6c94781e377d..546232106756 100644
> > --- a/hw/ppc/pnv_psi.c
> > +++ b/hw/ppc/pnv_psi.c
> > @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> > device_reset(DEVICE(dev));
> > }
> > +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> > +{
> > + PnvPsi *psi = PNV_PSI(dev);
> > +
> > + /* Default BAR for MMIO region */
> > + pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > +
> > + qemu_register_reset(pnv_psi_reset_handler, dev);
> > +}
> > +
> > static void pnv_psi_power8_instance_init(Object *obj)
> > {
> > Pnv8Psi *psi8 = PNV8_PSI(obj);
> > @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> > "psihb", PNV_PSIHB_SIZE);
> > - /* Default BAR for MMIO region */
> > - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > -
> > /* Default sources in XIVR */
> > for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> > uint8_t xivr = irq_to_xivr[i];
> > @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> > }
> > - qemu_register_reset(pnv_psi_reset_handler, dev);
> > + pnv_psi_realize(dev, errp);
> > }
> > static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> > @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> > memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> > "psihb", PNV9_PSIHB_SIZE);
> > - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > -
> > - qemu_register_reset(pnv_psi_reset_handler, dev);
> > + pnv_psi_realize(dev, errp);
> > }
> > static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Wed, 8 Jan 2020 11:54:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Greg,
> >
> > On 1/7/20 5:32 PM, Greg Kurz wrote:
> > > The proper way to do that would be to use device_class_set_parent_realize(),
> > > but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> > > pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> > > is fine for now.
> > >
> > > This should probably be achieved with a device realize hook in the
> > > PSI base class and device_class_set_parent_realize() in the children
> > > classes.
> >
I realize that this last paragraph is a leftover. First paragraph already
mentions device_class_set_parent_realize() as being the "proper way".
David,
Can you remove it in your tree ? No big deal if you can't.
> > Can you add a note explaining why the POWER10 PSI doesn't need it?
>
> For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
> doesn't arise.
>
This is correct and also a bit confusing, as proves Philippe's remark.
Maybe we should come up with a PnvXivePsiClass and specialize it for
POWER9 and POWER10.
> >
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > ---
> > > hw/ppc/pnv_psi.c | 19 ++++++++++++-------
> > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > > index 6c94781e377d..546232106756 100644
> > > --- a/hw/ppc/pnv_psi.c
> > > +++ b/hw/ppc/pnv_psi.c
> > > @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> > > device_reset(DEVICE(dev));
> > > }
> > > +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> > > +{
> > > + PnvPsi *psi = PNV_PSI(dev);
> > > +
> > > + /* Default BAR for MMIO region */
> > > + pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > +
> > > + qemu_register_reset(pnv_psi_reset_handler, dev);
> > > +}
> > > +
> > > static void pnv_psi_power8_instance_init(Object *obj)
> > > {
> > > Pnv8Psi *psi8 = PNV8_PSI(obj);
> > > @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > > memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> > > "psihb", PNV_PSIHB_SIZE);
> > > - /* Default BAR for MMIO region */
> > > - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > -
> > > /* Default sources in XIVR */
> > > for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> > > uint8_t xivr = irq_to_xivr[i];
> > > @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > > ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> > > }
> > > - qemu_register_reset(pnv_psi_reset_handler, dev);
> > > + pnv_psi_realize(dev, errp);
> > > }
> > > static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> > > @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> > > memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> > > "psihb", PNV9_PSIHB_SIZE);
> > > - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > -
> > > - qemu_register_reset(pnv_psi_reset_handler, dev);
> > > + pnv_psi_realize(dev, errp);
> > > }
> > > static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> > >
> > >
> >
>
On 1/8/20 11:58 AM, Greg Kurz wrote: > On Wed, 8 Jan 2020 11:54:53 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > >> On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote: >>> Hi Greg, >>> >>> On 1/7/20 5:32 PM, Greg Kurz wrote: >>>> The proper way to do that would be to use device_class_set_parent_realize(), >>>> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize >>>> pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely >>>> is fine for now. >>>> >>>> This should probably be achieved with a device realize hook in the >>>> PSI base class and device_class_set_parent_realize() in the children >>>> classes. >>> > > I realize that this last paragraph is a leftover. First paragraph already > mentions device_class_set_parent_realize() as being the "proper way". > > David, > > Can you remove it in your tree ? No big deal if you can't. > >>> Can you add a note explaining why the POWER10 PSI doesn't need it? >> >> For now, POWER10 uses the Pnv9PsiClass, I believe, so the question >> doesn't arise. >> > > This is correct and also a bit confusing, as proves Philippe's remark. > Maybe we should come up with a PnvXivePsiClass and specialize it for > POWER9 and POWER10. Yes. I think this is the way to go. P8 has a PSI device using the XICS interrupt interface. P9 and P10 use the XIVE interface. C.
On Wed, Jan 08, 2020 at 11:58:45AM +0100, Greg Kurz wrote:
> On Wed, 8 Jan 2020 11:54:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
> > > Hi Greg,
> > >
> > > On 1/7/20 5:32 PM, Greg Kurz wrote:
> > > > The proper way to do that would be to use device_class_set_parent_realize(),
> > > > but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> > > > pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> > > > is fine for now.
> > > >
> > > > This should probably be achieved with a device realize hook in the
> > > > PSI base class and device_class_set_parent_realize() in the children
> > > > classes.
> > >
>
> I realize that this last paragraph is a leftover. First paragraph already
> mentions device_class_set_parent_realize() as being the "proper way".
>
> David,
>
> Can you remove it in your tree ? No big deal if you can't.
Sorry, not any more - I've already sent the PR.
>
> > > Can you add a note explaining why the POWER10 PSI doesn't need it?
> >
> > For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
> > doesn't arise.
> >
>
> This is correct and also a bit confusing, as proves Philippe's remark.
> Maybe we should come up with a PnvXivePsiClass and specialize it for
> POWER9 and POWER10.
>
> > >
> > > >
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > > hw/ppc/pnv_psi.c | 19 ++++++++++++-------
> > > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > > > index 6c94781e377d..546232106756 100644
> > > > --- a/hw/ppc/pnv_psi.c
> > > > +++ b/hw/ppc/pnv_psi.c
> > > > @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> > > > device_reset(DEVICE(dev));
> > > > }
> > > > +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > + PnvPsi *psi = PNV_PSI(dev);
> > > > +
> > > > + /* Default BAR for MMIO region */
> > > > + pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > > +
> > > > + qemu_register_reset(pnv_psi_reset_handler, dev);
> > > > +}
> > > > +
> > > > static void pnv_psi_power8_instance_init(Object *obj)
> > > > {
> > > > Pnv8Psi *psi8 = PNV8_PSI(obj);
> > > > @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > > > memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> > > > "psihb", PNV_PSIHB_SIZE);
> > > > - /* Default BAR for MMIO region */
> > > > - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > > -
> > > > /* Default sources in XIVR */
> > > > for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> > > > uint8_t xivr = irq_to_xivr[i];
> > > > @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > > > ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> > > > }
> > > > - qemu_register_reset(pnv_psi_reset_handler, dev);
> > > > + pnv_psi_realize(dev, errp);
> > > > }
> > > > static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> > > > @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> > > > memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> > > > "psihb", PNV9_PSIHB_SIZE);
> > > > - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > > -
> > > > - qemu_register_reset(pnv_psi_reset_handler, dev);
> > > > + pnv_psi_realize(dev, errp);
> > > > }
> > > > static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> > > >
> > > >
> > >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Tue, Jan 07, 2020 at 05:32:46PM +0100, Greg Kurz wrote:
> The proper way to do that would be to use device_class_set_parent_realize(),
> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> is fine for now.
>
> This should probably be achieved with a device realize hook in the
> PSI base class and device_class_set_parent_realize() in the children
> classes.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Applied to ppc-for-5.0.
> ---
> hw/ppc/pnv_psi.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 6c94781e377d..546232106756 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> device_reset(DEVICE(dev));
> }
>
> +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> +{
> + PnvPsi *psi = PNV_PSI(dev);
> +
> + /* Default BAR for MMIO region */
> + pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> +
> + qemu_register_reset(pnv_psi_reset_handler, dev);
> +}
> +
> static void pnv_psi_power8_instance_init(Object *obj)
> {
> Pnv8Psi *psi8 = PNV8_PSI(obj);
> @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> "psihb", PNV_PSIHB_SIZE);
>
> - /* Default BAR for MMIO region */
> - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
> /* Default sources in XIVR */
> for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> uint8_t xivr = irq_to_xivr[i];
> @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> }
>
> - qemu_register_reset(pnv_psi_reset_handler, dev);
> + pnv_psi_realize(dev, errp);
> }
>
> static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> "psihb", PNV9_PSIHB_SIZE);
>
> - pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
> - qemu_register_reset(pnv_psi_reset_handler, dev);
> + pnv_psi_realize(dev, errp);
> }
>
> static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
© 2016 - 2026 Red Hat, Inc.