| Message ID | 20211228193806.1198496-2-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ppc/pnv: enable pnv-phb4 user devices | expand |
On 12/28/21 20:37, Daniel Henrique Barboza wrote: > When creating a pnv_phb3_root_port using the command line, the first > root port is created successfully, but the second fails with the > following error: > > qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3: > Can't add chassis slot, error -16 > > This error comes from the realize() function of its parent type, > rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails > with -EBUSY if there's an existing PCIESlot that has the same > chassis/slot value, regardless of being in a different bus. > > One way to prevent this error is simply set chassis and slot values in > the command line. However, since phb3 root buses only supports a single > root port, we can just get an unique chassis/slot value by checking > which root bus the pnv_phb3_root_port is going to be attached, get the > equivalent phb3 device and use its chip-id and index values, which are > guaranteed to be unique. I guess parent_realize() will fail if we add 2 root port devices under the same phb ? Thanks, C. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c > index 4e2d680d44..130d392b3e 100644 > --- a/hw/pci-host/pnv_phb3.c > +++ b/hw/pci-host/pnv_phb3.c > @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = { > static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp) > { > PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev); > + PCIDevice *pci = PCI_DEVICE(dev); > + PCIBus *bus = pci_get_bus(pci); > + PnvPHB3 *phb = NULL; > Error *local_err = NULL; > > + phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), > + TYPE_PNV_PHB3); > + > + if (!phb) { > + error_setg(errp, > +"pnv_phb3_root_port devices must be connected to pnv-phb3 buses"); > + return; > + } > + > + /* Set unique chassis/slot values for the root port */ > + qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id); > + qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id); > + > rpc->parent_realize(dev, &local_err); > if (local_err) { > error_propagate(errp, local_err); >
On 1/3/22 05:24, Cédric Le Goater wrote: > On 12/28/21 20:37, Daniel Henrique Barboza wrote: >> When creating a pnv_phb3_root_port using the command line, the first >> root port is created successfully, but the second fails with the >> following error: >> >> qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3: >> Can't add chassis slot, error -16 >> >> This error comes from the realize() function of its parent type, >> rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails >> with -EBUSY if there's an existing PCIESlot that has the same >> chassis/slot value, regardless of being in a different bus. >> >> One way to prevent this error is simply set chassis and slot values in >> the command line. However, since phb3 root buses only supports a single >> root port, we can just get an unique chassis/slot value by checking >> which root bus the pnv_phb3_root_port is going to be attached, get the >> equivalent phb3 device and use its chip-id and index values, which are >> guaranteed to be unique. > > I guess parent_realize() will fail if we add 2 root port devices under > the same phb ? If we change chassis/slot for each new pci root port the QEMU emulation will allow it. The problem is with skiboot which, at least according to the commit that introduced powernv9 support [1], does not support multiple PCIE devices in the same PHB: ---- No default device layout is provided and PCI devices can be added on any of the available PCIe Root Port (pcie.0 .. 2 of a Power9 chip) with address 0x0 as the firwware (skiboot) only accepts a single device per root port. ---- That said, I'm taking this information at face value. Perhaps this is a test worth doing to at least document this restriction more explicitly in the docs. [1] https://github.com/qemu/qemu/commit/4f9924c4d4cf9c039e247c5cdbbf71bce4e573c3 Thanks, Daniel > > Thanks, > > C. > > >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c >> index 4e2d680d44..130d392b3e 100644 >> --- a/hw/pci-host/pnv_phb3.c >> +++ b/hw/pci-host/pnv_phb3.c >> @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = { >> static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp) >> { >> PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev); >> + PCIDevice *pci = PCI_DEVICE(dev); >> + PCIBus *bus = pci_get_bus(pci); >> + PnvPHB3 *phb = NULL; >> Error *local_err = NULL; >> + phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), >> + TYPE_PNV_PHB3); >> + >> + if (!phb) { >> + error_setg(errp, >> +"pnv_phb3_root_port devices must be connected to pnv-phb3 buses"); >> + return; >> + } >> + >> + /* Set unique chassis/slot values for the root port */ >> + qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id); >> + qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id); >> + >> rpc->parent_realize(dev, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> >
On 1/3/22 05:24, Cédric Le Goater wrote: > On 12/28/21 20:37, Daniel Henrique Barboza wrote: >> When creating a pnv_phb3_root_port using the command line, the first >> root port is created successfully, but the second fails with the >> following error: >> >> qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3: >> Can't add chassis slot, error -16 >> >> This error comes from the realize() function of its parent type, >> rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails >> with -EBUSY if there's an existing PCIESlot that has the same >> chassis/slot value, regardless of being in a different bus. >> >> One way to prevent this error is simply set chassis and slot values in >> the command line. However, since phb3 root buses only supports a single >> root port, we can just get an unique chassis/slot value by checking >> which root bus the pnv_phb3_root_port is going to be attached, get the >> equivalent phb3 device and use its chip-id and index values, which are >> guaranteed to be unique. > > I guess parent_realize() will fail if we add 2 root port devices under > the same phb ? Yes. It will fail with an error similar to the one that happens with phb4 root ports: qemu-system-ppc64: -device pnv-phb4-root-port,bus=pnv-phb4-root-bus.0,slot=8: Bus 'pnv-phb4-root-bus.0' is full This happens because we're setting max_dev = 1 in the root bus class, in both cases: static void pnv_phb4_root_bus_class_init(ObjectClass *klass, void *data) { BusClass *k = BUS_CLASS(klass); /* * PHB4 has only a single root complex. Enforce the limit on the * parent bus */ k->max_dev = 1; } I also did a quick experiment with both. I set their root bus class max_dev to 2, then changed the code to allow adding a second root port in the PHB. The machine boots, which is good, but the OS doesn't see the extra root port in the output of 'lspci'. Same result for both. I can't see if this has to do with skiboot lacking support for it or something that we need to do in QEMU. For PHB4s, seeing how we've tied together the stack->phb->root port relationship throughout the code I can't say I'm surprised. Even if skiboot starts to allow multiple pci devices attached directly in the root complex we'll have to do some rework in the QEMU side to allow it. Thanks, Daniel > > Thanks, > > C. > > >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c >> index 4e2d680d44..130d392b3e 100644 >> --- a/hw/pci-host/pnv_phb3.c >> +++ b/hw/pci-host/pnv_phb3.c >> @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = { >> static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp) >> { >> PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev); >> + PCIDevice *pci = PCI_DEVICE(dev); >> + PCIBus *bus = pci_get_bus(pci); >> + PnvPHB3 *phb = NULL; >> Error *local_err = NULL; >> + phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), >> + TYPE_PNV_PHB3); >> + >> + if (!phb) { >> + error_setg(errp, >> +"pnv_phb3_root_port devices must be connected to pnv-phb3 buses"); >> + return; >> + } >> + >> + /* Set unique chassis/slot values for the root port */ >> + qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id); >> + qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id); >> + >> rpc->parent_realize(dev, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> >
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index 4e2d680d44..130d392b3e 100644 --- a/hw/pci-host/pnv_phb3.c +++ b/hw/pci-host/pnv_phb3.c @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = { static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp) { PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev); + PCIDevice *pci = PCI_DEVICE(dev); + PCIBus *bus = pci_get_bus(pci); + PnvPHB3 *phb = NULL; Error *local_err = NULL; + phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), + TYPE_PNV_PHB3); + + if (!phb) { + error_setg(errp, +"pnv_phb3_root_port devices must be connected to pnv-phb3 buses"); + return; + } + + /* Set unique chassis/slot values for the root port */ + qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id); + qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id); + rpc->parent_realize(dev, &local_err); if (local_err) { error_propagate(errp, local_err);
When creating a pnv_phb3_root_port using the command line, the first root port is created successfully, but the second fails with the following error: qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3: Can't add chassis slot, error -16 This error comes from the realize() function of its parent type, rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails with -EBUSY if there's an existing PCIESlot that has the same chassis/slot value, regardless of being in a different bus. One way to prevent this error is simply set chassis and slot values in the command line. However, since phb3 root buses only supports a single root port, we can just get an unique chassis/slot value by checking which root bus the pnv_phb3_root_port is going to be attached, get the equivalent phb3 device and use its chip-id and index values, which are guaranteed to be unique. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)