+ Ganesh
On 25/08/10 02:46PM, Cédric Le Goater wrote:
> + Glenn
> + Gautam
>
> On 8/10/25 12:45, Aditya Gupta wrote:
> > On 25/08/10 12:26PM, Aditya Gupta wrote:
> > > > <...snip...>
> > >
> > > About the error, seems xive2 always expecting powernv10 chip, I will
> > > have to rethink how should I use the same xive2 for powernv11.
> > >
> >
> > There's a type cast to Pnv10Chip in 'pnv_xive2_get_remote'.
> > The cast is only temporarily used to get the 'PnvXive2' object in the
> > Pnv10Chip.
> > It's the only place in hw/intc/pnv_xive2.c assuming Pnv10Chip object.
> >
> > Thinking to have a helper function to just return the 'PnvXive2' object
> > inside the chip, given a 'PnvChip'.
> >
> > Or the below change where we are adding another pointer in PnvChipClass:
> >
> > diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> > index e019cad5c14c..9832be5fd297 100644
> > --- a/hw/intc/pnv_xive2.c
> > +++ b/hw/intc/pnv_xive2.c
> > @@ -110,8 +110,8 @@ static PnvXive2 *pnv_xive2_get_remote(uint32_t
> > vsd_type, hwaddr fwd_addr)
> > int i;
> > for (i = 0; i < pnv->num_chips; i++) {
> > - Pnv10Chip *chip10 = PNV10_CHIP(pnv->chips[i]);
> > - PnvXive2 *xive = &chip10->xive;
> > + PnvChipClass *k = PNV_CHIP_GET_CLASS(pnv->chips[i]);
> > + PnvXive2 *xive = k->intc_get(pnv->chips[i]);
> > /*
> > * Is this the XIVE matching the forwarded VSD address is for
> > this
> >
> > Which one do you suggest ? Or should I look for another way ?
> >
> > I am preferring the first way to have a helper, but both ways look hacky.
>
> Any call to qdev_get_machine() in device model is at best
> a modeling shortcut, most likely it is a hack :
>
> /*
> * Remote access to INT controllers. HW uses MMIOs(?). For now, a simple
> * scan of all the chips INT controller is good enough.
> */
>
> So all these calls are suspicious :
>
> $ git grep qdev_get_machine hw/*/*pnv*
> hw/intc/pnv_xive2.c: PnvMachineState *pnv =
> PNV_MACHINE(qdev_get_machine());
> hw/pci-host/pnv_phb.c: PnvMachineState *pnv =
> PNV_MACHINE(qdev_get_machine());
> hw/pci-host/pnv_phb3.c: PnvMachineState *pnv =
> PNV_MACHINE(qdev_get_machine());
> hw/ppc/pnv.c: PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> hw/ppc/pnv.c: PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> hw/ppc/pnv_chiptod.c: PnvMachineState *pnv =
> PNV_MACHINE(qdev_get_machine());
> hw/ppc/pnv_chiptod.c: PnvMachineState *pnv =
> PNV_MACHINE(qdev_get_machine());
> hw/ppc/pnv_lpc.c: PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>
> In the particular case of XIVE2, the solution is to rework
> pnv_xive2_get_remote() like it was for P9. See b68147b7a5bf
> ("ppc/xive: Add support for the PC MMIOs").
>
Hi Cedric,
While I am working with XIVE engineers to get the pnv_xive2_get_remote()
reworked as suggested (since it's a bit more work due to multiple cases
of indirect/direct vst, nvg/nvc types in case of XIVE2), I would like
to propose below patch to have as an interim solution to unblock
the PowerNV11 support patches.
Please let me know if it looks good to you.
I am planning to have this as part of the pnv11 patches.
diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index e019cad5c14c..0663baab544c 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -110,8 +110,8 @@ static PnvXive2 *pnv_xive2_get_remote(uint32_t vsd_type,
hwaddr fwd_addr)
int i;
for (i = 0; i < pnv->num_chips; i++) {
- Pnv10Chip *chip10 = PNV10_CHIP(pnv->chips[i]);
- PnvXive2 *xive = &chip10->xive;
+ PnvChipClass *k = PNV_CHIP_GET_CLASS(pnv->chips[i]);
+ PnvXive2 *xive = PNV_XIVE2(k->intc_get(pnv->chips[i]));
/*
* Is this the XIVE matching the forwarded VSD address is for this
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 673bb54c6789..907e029c51eb 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1488,6 +1488,11 @@ static void pnv_chip_power10_intc_print_info(PnvChip
*chip, PowerPCCPU *cpu,
xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf);
}
+static void *pnv_chip_power10_intc_get(PnvChip *chip)
+{
+ return &PNV10_CHIP(chip)->xive;
+}
+
static void pnv_chip_power11_intc_create(PnvChip *chip, PowerPCCPU *cpu,
Error **errp)
{
@@ -1532,6 +1537,11 @@ static void pnv_chip_power11_intc_print_info(PnvChip
*chip, PowerPCCPU *cpu,
xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf);
}
+static void *pnv_chip_power11_intc_get(PnvChip *chip)
+{
+ return &PNV11_CHIP(chip)->xive;
+}
+
/*
* Allowed core identifiers on a POWER8 Processor Chip :
*
@@ -2716,6 +2726,7 @@ static void pnv_chip_power10_class_init(ObjectClass
*klass, const void *data)
k->intc_reset = pnv_chip_power10_intc_reset;
k->intc_destroy = pnv_chip_power10_intc_destroy;
k->intc_print_info = pnv_chip_power10_intc_print_info;
+ k->intc_get = pnv_chip_power10_intc_get;
k->isa_create = pnv_chip_power10_isa_create;
k->dt_populate = pnv_chip_power10_dt_populate;
k->pic_print_info = pnv_chip_power10_pic_print_info;
@@ -2749,6 +2760,7 @@ static void pnv_chip_power11_class_init(ObjectClass
*klass, const void *data)
k->intc_reset = pnv_chip_power11_intc_reset;
k->intc_destroy = pnv_chip_power11_intc_destroy;
k->intc_print_info = pnv_chip_power11_intc_print_info;
+ k->intc_get = pnv_chip_power11_intc_get;
k->isa_create = pnv_chip_power11_isa_create;
k->dt_populate = pnv_chip_power11_dt_populate;
k->pic_print_info = pnv_chip_power11_pic_print_info;
diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 6bd930f8b439..a5b8c49680d3 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -170,6 +170,7 @@ struct PnvChipClass {
void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu);
void (*intc_print_info)(PnvChip *chip, PowerPCCPU *cpu, GString *buf);
+ void* (*intc_get)(PnvChip *chip);
ISABus *(*isa_create)(PnvChip *chip, Error **errp);
void (*dt_populate)(PnvChip *chip, void *fdt);
void (*pic_print_info)(PnvChip *chip, GString *buf);
Thanks,
- Aditya G
