Message ID | 20220105212338.49899-18-danielhb413@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | user creatable pnv-phb3/pnv-phb4 devices | expand |
On 1/5/22 22:23, Daniel Henrique Barboza wrote: > This patch introduces pnv-phb4 user creatable devices that are created > in a similar manner as pnv-phb3 devices, allowing the user to interact > with the PHBs directly instead of creating PCI Express Controllers that > will create a certain amount of PHBs per controller index. > > First thing we need is to discover which stack will host the created > PHB, which is done by the new pnv_phb4_get_stack() function. During > pnv_phb4_realize() we'll inspect phb->stack to see if we're dealing with > an user creatable device or not. When using default settings, the > automatically created PHB4 devices will be realized with phb->stack > already assigned beforehand during PEC realize. In case we're dealing > with an user device, find its stack, set the PHB attributes based on the > stack it belongs and assign the PHB to the stack. > > The xscom stack initialization takes place in pnv_pec_stk_realize() when > using default settings, but at that point we aren't aware of any user > PHB4 devices that will belong to the stack. In that case we'll postpone > xscom stack init until the the end of pnv_phb4_realize(), after all the > memory mappings of the PHB are done. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/pci-host/pnv_phb4.c | 84 +++++++++++++++++++++++++++++++++++++- > hw/pci-host/pnv_phb4_pec.c | 12 +++--- > hw/ppc/pnv.c | 2 + > 3 files changed, 90 insertions(+), 8 deletions(-) > > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index 430a5c10f4..1c2334d491 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -1236,6 +1236,41 @@ static void pnv_phb4_instance_init(Object *obj) > object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE); > } > > +static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb, > + Error **errp) > +{ > + Pnv9Chip *chip9 = NULL; > + int chip_id = phb->chip_id; > + int index = phb->phb_id; > + int i, j; > + > + if (chip->num_pecs == 0) { assert() maybe ? > + /* Something weird happened. Bail out */ > + error_setg(errp, "chip id %d has no PCIE controllers", chip_id); > + return NULL; > + } > + > + chip9 = PNV9_CHIP(chip); > + > + for (i = 0; i < chip->num_pecs; i++) { > + /* > + * For each PEC, check the amount of stacks it supports > + * and see if the given phb4 index matches a stack. > + */ > + PnvPhb4PecState *pec = &chip9->pecs[i]; > + > + for (j = 0; j < pec->num_stacks; j++) { > + if (index == pnv_phb4_pec_get_phb_id(pec, j)) { > + return &pec->stacks[j]; > + } > + } > + } > + > + error_setg(errp, "pnv-phb4 index %d didn't match any existing PEC", > + chip_id); > + return NULL; > +} > + > static void pnv_phb4_realize(DeviceState *dev, Error **errp) > { > PnvPHB4 *phb = PNV_PHB4(dev); > @@ -1243,8 +1278,49 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) > XiveSource *xsrc = &phb->xsrc; > int nr_irqs; > char name[32]; > + PnvPhb4PecStack *stack = NULL; > + bool stack_init_xscom = false; > + Error *local_err = NULL; > > - assert(phb->stack); > + /* User created PHB */ > + if (!phb->stack) { > + PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine()); > + PnvChip *chip = pnv_get_chip(pnv, phb->chip_id); > + BusState *s; > + > + if (!chip) { > + error_setg(errp, "invalid chip id: %d", phb->chip_id); > + return; > + } > + > + stack = pnv_phb4_get_stack(chip, phb, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + object_property_set_int(OBJECT(phb), "index", > + phb->phb_id, &error_abort); > + > + pnv_phb4_set_stack_phb_props(stack, phb); > + > + /* Assign the phb to the stack */ > + stack->phb = phb; The stack object should check the validity of the stack->phb pointer always. > + > + /* > + * Reparent user created devices to the chip to build > + * correctly the device tree. > + */ > + pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id); > + > + s = qdev_get_parent_bus(DEVICE(chip)); > + if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) { > + error_propagate(errp, local_err); > + return; > + } > + > + stack_init_xscom = true; This looks wrong. > + } > > /* Set the "big_phb" flag */ > phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3; > @@ -1298,6 +1374,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) > pnv_phb4_update_xsrc(phb); > > phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs); > + > + if (stack_init_xscom) { > + pnv_pec_init_stack_xscom(stack); > + } Isn't it done under the pnv_pec_stk_realize() routine already ? > } > > static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge, > @@ -1347,7 +1427,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data) > dc->realize = pnv_phb4_realize; > device_class_set_props(dc, pnv_phb4_properties); > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > - dc->user_creatable = false; > + dc->user_creatable = true; > > xfc->notify = pnv_phb4_xive_notify; > } > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c > index 41c79d24c4..4417beb97d 100644 > --- a/hw/pci-host/pnv_phb4_pec.c > +++ b/hw/pci-host/pnv_phb4_pec.c > @@ -573,13 +573,13 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) > &pnv_pec_stk_pci_xscom_ops, stack, name, > PHB4_PEC_PCI_STK_REGS_COUNT); > > - /* PHB pass-through */ > - pnv_phb4_set_stack_phb_props(stack, stack->phb); > - if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) { > - return; > + /* > + * There is no guarantee that stack->phb will be available > + * at this point. > + */ > + if (stack->phb) { > + pnv_pec_init_stack_xscom(stack); > } > - > - pnv_pec_init_stack_xscom(stack); This looks wrong also. I don't understand why. Thanks, C. > } > > static Property pnv_pec_stk_properties[] = { > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index fe7e67e73a..837146a2fb 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data) > pmc->compat = compat; > pmc->compat_size = sizeof(compat); > pmc->dt_power_mgt = pnv_dt_power_mgt; > + > + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4); > } > > static void pnv_machine_power10_class_init(ObjectClass *oc, void *data) >
On 1/6/22 11:49, Cédric Le Goater wrote: > On 1/5/22 22:23, Daniel Henrique Barboza wrote: >> This patch introduces pnv-phb4 user creatable devices that are created >> in a similar manner as pnv-phb3 devices, allowing the user to interact >> with the PHBs directly instead of creating PCI Express Controllers that >> will create a certain amount of PHBs per controller index. >> >> First thing we need is to discover which stack will host the created >> PHB, which is done by the new pnv_phb4_get_stack() function. During >> pnv_phb4_realize() we'll inspect phb->stack to see if we're dealing with >> an user creatable device or not. When using default settings, the >> automatically created PHB4 devices will be realized with phb->stack >> already assigned beforehand during PEC realize. In case we're dealing >> with an user device, find its stack, set the PHB attributes based on the >> stack it belongs and assign the PHB to the stack. >> >> The xscom stack initialization takes place in pnv_pec_stk_realize() when >> using default settings, but at that point we aren't aware of any user >> PHB4 devices that will belong to the stack. In that case we'll postpone >> xscom stack init until the the end of pnv_phb4_realize(), after all the >> memory mappings of the PHB are done. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/pci-host/pnv_phb4.c | 84 +++++++++++++++++++++++++++++++++++++- >> hw/pci-host/pnv_phb4_pec.c | 12 +++--- >> hw/ppc/pnv.c | 2 + >> 3 files changed, 90 insertions(+), 8 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c >> index 430a5c10f4..1c2334d491 100644 >> --- a/hw/pci-host/pnv_phb4.c >> +++ b/hw/pci-host/pnv_phb4.c >> @@ -1236,6 +1236,41 @@ static void pnv_phb4_instance_init(Object *obj) >> object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE); >> } >> +static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb, >> + Error **errp) >> +{ >> + Pnv9Chip *chip9 = NULL; >> + int chip_id = phb->chip_id; >> + int index = phb->phb_id; >> + int i, j; >> + >> + if (chip->num_pecs == 0) { > > assert() maybe ? > > >> + /* Something weird happened. Bail out */ >> + error_setg(errp, "chip id %d has no PCIE controllers", chip_id); >> + return NULL; >> + } >> + >> + chip9 = PNV9_CHIP(chip); >> + >> + for (i = 0; i < chip->num_pecs; i++) { >> + /* >> + * For each PEC, check the amount of stacks it supports >> + * and see if the given phb4 index matches a stack. >> + */ >> + PnvPhb4PecState *pec = &chip9->pecs[i]; >> + >> + for (j = 0; j < pec->num_stacks; j++) { >> + if (index == pnv_phb4_pec_get_phb_id(pec, j)) { >> + return &pec->stacks[j]; >> + } >> + } >> + } >> + >> + error_setg(errp, "pnv-phb4 index %d didn't match any existing PEC", >> + chip_id); >> + return NULL; >> +} >> + >> static void pnv_phb4_realize(DeviceState *dev, Error **errp) >> { >> PnvPHB4 *phb = PNV_PHB4(dev); >> @@ -1243,8 +1278,49 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) >> XiveSource *xsrc = &phb->xsrc; >> int nr_irqs; >> char name[32]; >> + PnvPhb4PecStack *stack = NULL; >> + bool stack_init_xscom = false; >> + Error *local_err = NULL; >> - assert(phb->stack); >> + /* User created PHB */ >> + if (!phb->stack) { >> + PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine()); >> + PnvChip *chip = pnv_get_chip(pnv, phb->chip_id); >> + BusState *s; >> + >> + if (!chip) { >> + error_setg(errp, "invalid chip id: %d", phb->chip_id); >> + return; >> + } >> + >> + stack = pnv_phb4_get_stack(chip, phb, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + >> + object_property_set_int(OBJECT(phb), "index", >> + phb->phb_id, &error_abort); >> + >> + pnv_phb4_set_stack_phb_props(stack, phb); >> + >> + /* Assign the phb to the stack */ >> + stack->phb = phb; > > The stack object should check the validity of the stack->phb pointer always. What do you mean by "check the validity"? Thanks, Daniel > >> + >> + /* >> + * Reparent user created devices to the chip to build >> + * correctly the device tree. >> + */ >> + pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id); >> + >> + s = qdev_get_parent_bus(DEVICE(chip)); >> + if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + >> + stack_init_xscom = true; > > This looks wrong. > >> + } >> /* Set the "big_phb" flag */ >> phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3; >> @@ -1298,6 +1374,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) >> pnv_phb4_update_xsrc(phb); >> phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs); >> + >> + if (stack_init_xscom) { >> + pnv_pec_init_stack_xscom(stack); >> + } > > Isn't it done under the pnv_pec_stk_realize() routine already ? > >> } >> static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge, >> @@ -1347,7 +1427,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data) >> dc->realize = pnv_phb4_realize; >> device_class_set_props(dc, pnv_phb4_properties); >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> - dc->user_creatable = false; >> + dc->user_creatable = true; >> xfc->notify = pnv_phb4_xive_notify; >> } >> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c >> index 41c79d24c4..4417beb97d 100644 >> --- a/hw/pci-host/pnv_phb4_pec.c >> +++ b/hw/pci-host/pnv_phb4_pec.c >> @@ -573,13 +573,13 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) >> &pnv_pec_stk_pci_xscom_ops, stack, name, >> PHB4_PEC_PCI_STK_REGS_COUNT); >> - /* PHB pass-through */ >> - pnv_phb4_set_stack_phb_props(stack, stack->phb); >> - if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) { >> - return; >> + /* >> + * There is no guarantee that stack->phb will be available >> + * at this point. >> + */ >> + if (stack->phb) { >> + pnv_pec_init_stack_xscom(stack); >> } >> - >> - pnv_pec_init_stack_xscom(stack); > > > This looks wrong also. I don't understand why. > > Thanks, > > C. > > >> } >> static Property pnv_pec_stk_properties[] = { >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index fe7e67e73a..837146a2fb 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data) >> pmc->compat = compat; >> pmc->compat_size = sizeof(compat); >> pmc->dt_power_mgt = pnv_dt_power_mgt; >> + >> + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4); >> } >> static void pnv_machine_power10_class_init(ObjectClass *oc, void *data) >> >
>>> + object_property_set_int(OBJECT(phb), "index", >>> + phb->phb_id, &error_abort); >>> + >>> + pnv_phb4_set_stack_phb_props(stack, phb); >>> + >>> + /* Assign the phb to the stack */ >>> + stack->phb = phb; >> >> The stack object should check the validity of the stack->phb pointer always. > > > What do you mean by "check the validity"? > I am thinking of the usage of 'stack->phb', for instance in the routine pnv_phb4_update_regions(). We should add an assert there. Your changes seem to cleanup the stack <-> phb relation quite a lot. Which is good. Thanks, C.
On 1/8/22 08:11, Cédric Le Goater wrote: >>>> + object_property_set_int(OBJECT(phb), "index", >>>> + phb->phb_id, &error_abort); >>>> + >>>> + pnv_phb4_set_stack_phb_props(stack, phb); >>>> + >>>> + /* Assign the phb to the stack */ >>>> + stack->phb = phb; >>> >>> The stack object should check the validity of the stack->phb pointer always. >> >> >> What do you mean by "check the validity"? >> > > I am thinking of the usage of 'stack->phb', for instance in the routine > pnv_phb4_update_regions(). We should add an assert there. Oh ok. I'll add an assert in this point (and in the code above that where you also pointed it out. And about pnv_phb4_update_regions(), v3 will need to handle it but not with an assert but with a phb != NULL verification to become a NO-OP if phb is NULL. It's an easier and less intrusive fix than trying to change the order of the XSCOM initialization in stk_realize(). I will (hopefully) explain it better in the v3. Thanks, Daniel > > Your changes seem to cleanup the stack <-> phb relation quite a lot. Which > is good. > > Thanks, > > C.
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index 430a5c10f4..1c2334d491 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -1236,6 +1236,41 @@ static void pnv_phb4_instance_init(Object *obj) object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE); } +static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb, + Error **errp) +{ + Pnv9Chip *chip9 = NULL; + int chip_id = phb->chip_id; + int index = phb->phb_id; + int i, j; + + if (chip->num_pecs == 0) { + /* Something weird happened. Bail out */ + error_setg(errp, "chip id %d has no PCIE controllers", chip_id); + return NULL; + } + + chip9 = PNV9_CHIP(chip); + + for (i = 0; i < chip->num_pecs; i++) { + /* + * For each PEC, check the amount of stacks it supports + * and see if the given phb4 index matches a stack. + */ + PnvPhb4PecState *pec = &chip9->pecs[i]; + + for (j = 0; j < pec->num_stacks; j++) { + if (index == pnv_phb4_pec_get_phb_id(pec, j)) { + return &pec->stacks[j]; + } + } + } + + error_setg(errp, "pnv-phb4 index %d didn't match any existing PEC", + chip_id); + return NULL; +} + static void pnv_phb4_realize(DeviceState *dev, Error **errp) { PnvPHB4 *phb = PNV_PHB4(dev); @@ -1243,8 +1278,49 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) XiveSource *xsrc = &phb->xsrc; int nr_irqs; char name[32]; + PnvPhb4PecStack *stack = NULL; + bool stack_init_xscom = false; + Error *local_err = NULL; - assert(phb->stack); + /* User created PHB */ + if (!phb->stack) { + PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine()); + PnvChip *chip = pnv_get_chip(pnv, phb->chip_id); + BusState *s; + + if (!chip) { + error_setg(errp, "invalid chip id: %d", phb->chip_id); + return; + } + + stack = pnv_phb4_get_stack(chip, phb, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + object_property_set_int(OBJECT(phb), "index", + phb->phb_id, &error_abort); + + pnv_phb4_set_stack_phb_props(stack, phb); + + /* Assign the phb to the stack */ + stack->phb = phb; + + /* + * Reparent user created devices to the chip to build + * correctly the device tree. + */ + pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id); + + s = qdev_get_parent_bus(DEVICE(chip)); + if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) { + error_propagate(errp, local_err); + return; + } + + stack_init_xscom = true; + } /* Set the "big_phb" flag */ phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3; @@ -1298,6 +1374,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) pnv_phb4_update_xsrc(phb); phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs); + + if (stack_init_xscom) { + pnv_pec_init_stack_xscom(stack); + } } static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge, @@ -1347,7 +1427,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data) dc->realize = pnv_phb4_realize; device_class_set_props(dc, pnv_phb4_properties); set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); - dc->user_creatable = false; + dc->user_creatable = true; xfc->notify = pnv_phb4_xive_notify; } diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c index 41c79d24c4..4417beb97d 100644 --- a/hw/pci-host/pnv_phb4_pec.c +++ b/hw/pci-host/pnv_phb4_pec.c @@ -573,13 +573,13 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) &pnv_pec_stk_pci_xscom_ops, stack, name, PHB4_PEC_PCI_STK_REGS_COUNT); - /* PHB pass-through */ - pnv_phb4_set_stack_phb_props(stack, stack->phb); - if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) { - return; + /* + * There is no guarantee that stack->phb will be available + * at this point. + */ + if (stack->phb) { + pnv_pec_init_stack_xscom(stack); } - - pnv_pec_init_stack_xscom(stack); } static Property pnv_pec_stk_properties[] = { diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index fe7e67e73a..837146a2fb 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data) pmc->compat = compat; pmc->compat_size = sizeof(compat); pmc->dt_power_mgt = pnv_dt_power_mgt; + + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4); } static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
This patch introduces pnv-phb4 user creatable devices that are created in a similar manner as pnv-phb3 devices, allowing the user to interact with the PHBs directly instead of creating PCI Express Controllers that will create a certain amount of PHBs per controller index. First thing we need is to discover which stack will host the created PHB, which is done by the new pnv_phb4_get_stack() function. During pnv_phb4_realize() we'll inspect phb->stack to see if we're dealing with an user creatable device or not. When using default settings, the automatically created PHB4 devices will be realized with phb->stack already assigned beforehand during PEC realize. In case we're dealing with an user device, find its stack, set the PHB attributes based on the stack it belongs and assign the PHB to the stack. The xscom stack initialization takes place in pnv_pec_stk_realize() when using default settings, but at that point we aren't aware of any user PHB4 devices that will belong to the stack. In that case we'll postpone xscom stack init until the the end of pnv_phb4_realize(), after all the memory mappings of the PHB are done. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb4.c | 84 +++++++++++++++++++++++++++++++++++++- hw/pci-host/pnv_phb4_pec.c | 12 +++--- hw/ppc/pnv.c | 2 + 3 files changed, 90 insertions(+), 8 deletions(-)