| Message ID | 20220531214917.31668-5-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | powernv: introduce pnv-phb base/proxy devices | expand |
On 31/05/2022 22:49, Daniel Henrique Barboza wrote: > We need a handful of changes that needs to be done in a single swoop to > turn PnvPHB3 into a PnvPHB backend. > > In the PnvPHB3, since the PnvPHB device implements PCIExpressHost and > will hold the PCI bus, change PnvPHB3 parent to TYPE_DEVICE. There are a > couple of instances in pnv_phb3.c that needs to access the PCI bus, so a > phb_base pointer is added to allow access to the parent PnvPHB. The > PnvPHB3 root port will now be connected to a PnvPHB object. > > In pnv.c, the powernv8 machine chip8 will now hold an array of PnvPHB > objects. pnv_get_phb3_child() needs to be adapted to return the PnvPHB3 > backend from the PnvPHB child. A global property is added in > pnv_machine_power8_class_init() to ensure that all PnvPHBs are created > with phb->version = 3. > > After all these changes we're still able to boot a powernv8 machine with > default settings. The real gain will come with user created PnvPHB > devices, coming up next. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/pci-host/pnv_phb3.c | 29 ++++++++--------------------- > hw/ppc/pnv.c | 21 +++++++++++++++++---- > include/hw/pci-host/pnv_phb3.h | 5 ++++- > include/hw/ppc/pnv.h | 3 ++- > 4 files changed, 31 insertions(+), 27 deletions(-) > > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c > index 60584e2aae..a39aa0e8c4 100644 > --- a/hw/pci-host/pnv_phb3.c > +++ b/hw/pci-host/pnv_phb3.c > @@ -11,6 +11,7 @@ > #include "qapi/visitor.h" > #include "qapi/error.h" > #include "hw/pci-host/pnv_phb3_regs.h" > +#include "hw/pci-host/pnv_phb.h" > #include "hw/pci-host/pnv_phb3.h" > #include "hw/pci/pcie_host.h" > #include "hw/pci/pcie_port.h" > @@ -26,7 +27,7 @@ > > static PCIDevice *pnv_phb3_find_cfg_dev(PnvPHB3 *phb) > { > - PCIHostState *pci = PCI_HOST_BRIDGE(phb); > + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); > uint64_t addr = phb->regs[PHB_CONFIG_ADDRESS >> 3]; > uint8_t bus, devfn; > > @@ -590,7 +591,7 @@ void pnv_phb3_reg_write(void *opaque, hwaddr off, uint64_t val, unsigned size) > uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size) > { > PnvPHB3 *phb = opaque; > - PCIHostState *pci = PCI_HOST_BRIDGE(phb); > + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); > uint64_t val; > > if ((off & 0xfffc) == PHB_CONFIG_DATA) { > @@ -1057,8 +1058,6 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp) > "phb3-regs", 0x1000); > > pnv_phb3_bus_init(dev, phb); > - > - pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT); > } > > void pnv_phb3_update_regions(PnvPHB3 *phb) > @@ -1083,38 +1082,26 @@ void pnv_phb3_update_regions(PnvPHB3 *phb) > pnv_phb3_check_all_m64s(phb); > } > > -static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge, > - PCIBus *rootbus) > -{ > - PnvPHB3 *phb = PNV_PHB3(host_bridge); > - > - snprintf(phb->bus_path, sizeof(phb->bus_path), "00%02x:%02x", > - phb->chip_id, phb->phb_id); > - return phb->bus_path; > -} > - > static Property pnv_phb3_properties[] = { > DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0), > DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0), > DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *), > + DEFINE_PROP_LINK("phb-base", PnvPHB3, phb_base, TYPE_PNV_PHB, PnvPHB *), > DEFINE_PROP_END_OF_LIST(), > }; > > static void pnv_phb3_class_init(ObjectClass *klass, void *data) > { > - PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass); > DeviceClass *dc = DEVICE_CLASS(klass); > > - hc->root_bus_path = pnv_phb3_root_bus_path; > dc->realize = pnv_phb3_realize; > device_class_set_props(dc, pnv_phb3_properties); > - set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > dc->user_creatable = false; > } > > static const TypeInfo pnv_phb3_type_info = { > .name = TYPE_PNV_PHB3, > - .parent = TYPE_PCIE_HOST_BRIDGE, > + .parent = TYPE_DEVICE, > .instance_size = sizeof(PnvPHB3), > .class_init = pnv_phb3_class_init, > .instance_init = pnv_phb3_instance_init, > @@ -1146,11 +1133,11 @@ 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; > + PnvPHB *phb = NULL; > Error *local_err = NULL; > > - phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), > - TYPE_PNV_PHB3); > + phb = (PnvPHB *) object_dynamic_cast(OBJECT(bus->qbus.parent), > + TYPE_PNV_PHB); I realize that this has come from existing code, however these days there generally isn't a good reason for anything to access bus->qbus directly (and C casts without a QOM macro often need a closer look). I'm also not convinced by the use of object_dynamic_cast() here either. Could this be rewritten as something like: phb = PNV_PHB(PNV_PHB3_ROOT_PORT(dev)->phb_base); > if (!phb) { > error_setg(errp, > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index aaf4d241c3..6cd0af9adf 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -43,6 +43,7 @@ > #include "hw/ipmi/ipmi.h" > #include "target/ppc/mmu-hash64.h" > #include "hw/pci/msi.h" > +#include "hw/pci-host/pnv_phb.h" > > #include "hw/ppc/xics.h" > #include "hw/qdev-properties.h" > @@ -654,7 +655,13 @@ static ISABus *pnv_isa_create(PnvChip *chip, Error **errp) > > static PnvPHB3 *pnv_get_phb3_child(Object *child) > { > - return (PnvPHB3 *)object_dynamic_cast(child, TYPE_PNV_PHB3); > + PnvPHB *phb = (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB); And here, assuming child is of type TYPE_PNV_PHB then as it is already known at compile time so I think this could become just: PnvPHB *phb = PNV_PHB(child); > + if (!phb) { > + return NULL; > + } > + > + return (PnvPHB3 *)phb->backend; > } > > static int pnv_chip_power8_pic_print_info_child(Object *child, void *opaque) > @@ -1160,7 +1167,7 @@ static void pnv_chip_power8_instance_init(Object *obj) > chip8->num_phbs = pcc->num_phbs; > > for (i = 0; i < chip8->num_phbs; i++) { > - object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3); > + object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB); > } > > } > @@ -1282,9 +1289,9 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp) > memory_region_add_subregion(get_system_memory(), PNV_HOMER_BASE(chip), > &chip8->homer.regs); > > - /* PHB3 controllers */ > + /* PHB controllers */ > for (i = 0; i < chip8->num_phbs; i++) { > - PnvPHB3 *phb = &chip8->phbs[i]; > + PnvPHB *phb = &chip8->phbs[i]; > > object_property_set_int(OBJECT(phb), "index", i, &error_fatal); > object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id, > @@ -1957,6 +1964,7 @@ static int pnv_ics_get_child(Object *child, void *opaque) > args->ics = ICS(&phb3->msis); > } > } > + > return args->ics ? 1 : 0; > } > > @@ -2112,8 +2120,13 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > > + static GlobalProperty phb_compat[] = { > + { TYPE_PNV_PHB, "version", "3" }, > + }; > + > mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); > + compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat)); > > xic->icp_get = pnv_icp_get; > xic->ics_get = pnv_ics_get; > diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h > index 1375f18fc1..3b9ff1096a 100644 > --- a/include/hw/pci-host/pnv_phb3.h > +++ b/include/hw/pci-host/pnv_phb3.h > @@ -14,6 +14,7 @@ > #include "hw/pci/pcie_port.h" > #include "hw/ppc/xics.h" > #include "qom/object.h" > +#include "hw/pci-host/pnv_phb.h" > > typedef struct PnvPHB3 PnvPHB3; > typedef struct PnvChip PnvChip; > @@ -127,7 +128,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB3, PNV_PHB3) > #define PCI_MMIO_TOTAL_SIZE (0x1ull << 60) > > struct PnvPHB3 { > - PCIExpressHost parent_obj; > + DeviceState parent; > + > + PnvPHB *phb_base; > > uint32_t chip_id; > uint32_t phb_id; > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 86cb7d7f97..4595db418e 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -32,6 +32,7 @@ > #include "hw/ppc/pnv_core.h" > #include "hw/pci-host/pnv_phb3.h" > #include "hw/pci-host/pnv_phb4.h" > +#include "hw/pci-host/pnv_phb.h" > #include "qom/object.h" > > #define TYPE_PNV_CHIP "pnv-chip" > @@ -80,7 +81,7 @@ struct Pnv8Chip { > PnvHomer homer; > > #define PNV8_CHIP_PHB3_MAX 4 > - PnvPHB3 phbs[PNV8_CHIP_PHB3_MAX]; > + PnvPHB phbs[PNV8_CHIP_PHB3_MAX]; > uint32_t num_phbs; > > XICSFabric *xics; ATB, Mark.
On 6/2/22 04:56, Mark Cave-Ayland wrote: > On 31/05/2022 22:49, Daniel Henrique Barboza wrote: > >> We need a handful of changes that needs to be done in a single swoop to >> turn PnvPHB3 into a PnvPHB backend. >> >> In the PnvPHB3, since the PnvPHB device implements PCIExpressHost and >> will hold the PCI bus, change PnvPHB3 parent to TYPE_DEVICE. There are a >> couple of instances in pnv_phb3.c that needs to access the PCI bus, so a >> phb_base pointer is added to allow access to the parent PnvPHB. The >> PnvPHB3 root port will now be connected to a PnvPHB object. >> >> In pnv.c, the powernv8 machine chip8 will now hold an array of PnvPHB >> objects. pnv_get_phb3_child() needs to be adapted to return the PnvPHB3 >> backend from the PnvPHB child. A global property is added in >> pnv_machine_power8_class_init() to ensure that all PnvPHBs are created >> with phb->version = 3. >> >> After all these changes we're still able to boot a powernv8 machine with >> default settings. The real gain will come with user created PnvPHB >> devices, coming up next. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/pci-host/pnv_phb3.c | 29 ++++++++--------------------- >> hw/ppc/pnv.c | 21 +++++++++++++++++---- >> include/hw/pci-host/pnv_phb3.h | 5 ++++- >> include/hw/ppc/pnv.h | 3 ++- >> 4 files changed, 31 insertions(+), 27 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c >> index 60584e2aae..a39aa0e8c4 100644 >> --- a/hw/pci-host/pnv_phb3.c >> +++ b/hw/pci-host/pnv_phb3.c >> @@ -11,6 +11,7 @@ >> #include "qapi/visitor.h" >> #include "qapi/error.h" >> #include "hw/pci-host/pnv_phb3_regs.h" >> +#include "hw/pci-host/pnv_phb.h" >> #include "hw/pci-host/pnv_phb3.h" >> #include "hw/pci/pcie_host.h" >> #include "hw/pci/pcie_port.h" >> @@ -26,7 +27,7 @@ >> static PCIDevice *pnv_phb3_find_cfg_dev(PnvPHB3 *phb) >> { >> - PCIHostState *pci = PCI_HOST_BRIDGE(phb); >> + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); >> uint64_t addr = phb->regs[PHB_CONFIG_ADDRESS >> 3]; >> uint8_t bus, devfn; >> @@ -590,7 +591,7 @@ void pnv_phb3_reg_write(void *opaque, hwaddr off, uint64_t val, unsigned size) >> uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size) >> { >> PnvPHB3 *phb = opaque; >> - PCIHostState *pci = PCI_HOST_BRIDGE(phb); >> + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); >> uint64_t val; >> if ((off & 0xfffc) == PHB_CONFIG_DATA) { >> @@ -1057,8 +1058,6 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp) >> "phb3-regs", 0x1000); >> pnv_phb3_bus_init(dev, phb); >> - >> - pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT); >> } >> void pnv_phb3_update_regions(PnvPHB3 *phb) >> @@ -1083,38 +1082,26 @@ void pnv_phb3_update_regions(PnvPHB3 *phb) >> pnv_phb3_check_all_m64s(phb); >> } >> -static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge, >> - PCIBus *rootbus) >> -{ >> - PnvPHB3 *phb = PNV_PHB3(host_bridge); >> - >> - snprintf(phb->bus_path, sizeof(phb->bus_path), "00%02x:%02x", >> - phb->chip_id, phb->phb_id); >> - return phb->bus_path; >> -} >> - >> static Property pnv_phb3_properties[] = { >> DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0), >> DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0), >> DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *), >> + DEFINE_PROP_LINK("phb-base", PnvPHB3, phb_base, TYPE_PNV_PHB, PnvPHB *), >> DEFINE_PROP_END_OF_LIST(), >> }; >> static void pnv_phb3_class_init(ObjectClass *klass, void *data) >> { >> - PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass); >> DeviceClass *dc = DEVICE_CLASS(klass); >> - hc->root_bus_path = pnv_phb3_root_bus_path; >> dc->realize = pnv_phb3_realize; >> device_class_set_props(dc, pnv_phb3_properties); >> - set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> dc->user_creatable = false; >> } >> static const TypeInfo pnv_phb3_type_info = { >> .name = TYPE_PNV_PHB3, >> - .parent = TYPE_PCIE_HOST_BRIDGE, >> + .parent = TYPE_DEVICE, >> .instance_size = sizeof(PnvPHB3), >> .class_init = pnv_phb3_class_init, >> .instance_init = pnv_phb3_instance_init, >> @@ -1146,11 +1133,11 @@ 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; >> + PnvPHB *phb = NULL; >> Error *local_err = NULL; >> - phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), >> - TYPE_PNV_PHB3); >> + phb = (PnvPHB *) object_dynamic_cast(OBJECT(bus->qbus.parent), >> + TYPE_PNV_PHB); > > I realize that this has come from existing code, however these days there generally isn't a good reason for anything to access bus->qbus directly (and C casts without a QOM macro often need a closer look). I'm also not convinced by the use of object_dynamic_cast() here either. Could this be rewritten as something like: > > phb = PNV_PHB(PNV_PHB3_ROOT_PORT(dev)->phb_base); Just checked. At this moment we can't because we don't declare a PNV_PHB3_ROOT_PORT() macro for this device. Yes, we're missing a OBJECT_DECLARE_SIMPLE_TYPE() for it. Same thing for PnvPHB4RootPort. I can add the missing PNV_PHB3_ROOT_PORT() macro and do this change. Or I can make a comment mentioning that this code can be improved but it's going to be removed shortly and this will be amended appropriately in the pnv-phb-root-port code. What do you think? I'm fine with both alternatives. Thanks, Daniel > >> if (!phb) { >> error_setg(errp, >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index aaf4d241c3..6cd0af9adf 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -43,6 +43,7 @@ >> #include "hw/ipmi/ipmi.h" >> #include "target/ppc/mmu-hash64.h" >> #include "hw/pci/msi.h" >> +#include "hw/pci-host/pnv_phb.h" >> #include "hw/ppc/xics.h" >> #include "hw/qdev-properties.h" >> @@ -654,7 +655,13 @@ static ISABus *pnv_isa_create(PnvChip *chip, Error **errp) >> static PnvPHB3 *pnv_get_phb3_child(Object *child) >> { >> - return (PnvPHB3 *)object_dynamic_cast(child, TYPE_PNV_PHB3); >> + PnvPHB *phb = (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB); > > And here, assuming child is of type TYPE_PNV_PHB then as it is already known at compile time so I think this could become just: > > PnvPHB *phb = PNV_PHB(child); > >> + if (!phb) { >> + return NULL; >> + } >> + >> + return (PnvPHB3 *)phb->backend; >> } >> static int pnv_chip_power8_pic_print_info_child(Object *child, void *opaque) >> @@ -1160,7 +1167,7 @@ static void pnv_chip_power8_instance_init(Object *obj) >> chip8->num_phbs = pcc->num_phbs; >> for (i = 0; i < chip8->num_phbs; i++) { >> - object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3); >> + object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB); >> } >> } >> @@ -1282,9 +1289,9 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp) >> memory_region_add_subregion(get_system_memory(), PNV_HOMER_BASE(chip), >> &chip8->homer.regs); >> - /* PHB3 controllers */ >> + /* PHB controllers */ >> for (i = 0; i < chip8->num_phbs; i++) { >> - PnvPHB3 *phb = &chip8->phbs[i]; >> + PnvPHB *phb = &chip8->phbs[i]; >> object_property_set_int(OBJECT(phb), "index", i, &error_fatal); >> object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id, >> @@ -1957,6 +1964,7 @@ static int pnv_ics_get_child(Object *child, void *opaque) >> args->ics = ICS(&phb3->msis); >> } >> } >> + >> return args->ics ? 1 : 0; >> } >> @@ -2112,8 +2120,13 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) >> PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); >> static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; >> + static GlobalProperty phb_compat[] = { >> + { TYPE_PNV_PHB, "version", "3" }, >> + }; >> + >> mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; >> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); >> + compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat)); >> xic->icp_get = pnv_icp_get; >> xic->ics_get = pnv_ics_get; >> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h >> index 1375f18fc1..3b9ff1096a 100644 >> --- a/include/hw/pci-host/pnv_phb3.h >> +++ b/include/hw/pci-host/pnv_phb3.h >> @@ -14,6 +14,7 @@ >> #include "hw/pci/pcie_port.h" >> #include "hw/ppc/xics.h" >> #include "qom/object.h" >> +#include "hw/pci-host/pnv_phb.h" >> typedef struct PnvPHB3 PnvPHB3; >> typedef struct PnvChip PnvChip; >> @@ -127,7 +128,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB3, PNV_PHB3) >> #define PCI_MMIO_TOTAL_SIZE (0x1ull << 60) >> struct PnvPHB3 { >> - PCIExpressHost parent_obj; >> + DeviceState parent; >> + >> + PnvPHB *phb_base; >> uint32_t chip_id; >> uint32_t phb_id; >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h >> index 86cb7d7f97..4595db418e 100644 >> --- a/include/hw/ppc/pnv.h >> +++ b/include/hw/ppc/pnv.h >> @@ -32,6 +32,7 @@ >> #include "hw/ppc/pnv_core.h" >> #include "hw/pci-host/pnv_phb3.h" >> #include "hw/pci-host/pnv_phb4.h" >> +#include "hw/pci-host/pnv_phb.h" >> #include "qom/object.h" >> #define TYPE_PNV_CHIP "pnv-chip" >> @@ -80,7 +81,7 @@ struct Pnv8Chip { >> PnvHomer homer; >> #define PNV8_CHIP_PHB3_MAX 4 >> - PnvPHB3 phbs[PNV8_CHIP_PHB3_MAX]; >> + PnvPHB phbs[PNV8_CHIP_PHB3_MAX]; >> uint32_t num_phbs; >> XICSFabric *xics; > > > ATB, > > Mark.
On 03/06/2022 21:30, Daniel Henrique Barboza wrote: > On 6/2/22 04:56, Mark Cave-Ayland wrote: >> On 31/05/2022 22:49, Daniel Henrique Barboza wrote: >> >>> We need a handful of changes that needs to be done in a single swoop to >>> turn PnvPHB3 into a PnvPHB backend. >>> >>> In the PnvPHB3, since the PnvPHB device implements PCIExpressHost and >>> will hold the PCI bus, change PnvPHB3 parent to TYPE_DEVICE. There are a >>> couple of instances in pnv_phb3.c that needs to access the PCI bus, so a >>> phb_base pointer is added to allow access to the parent PnvPHB. The >>> PnvPHB3 root port will now be connected to a PnvPHB object. >>> >>> In pnv.c, the powernv8 machine chip8 will now hold an array of PnvPHB >>> objects. pnv_get_phb3_child() needs to be adapted to return the PnvPHB3 >>> backend from the PnvPHB child. A global property is added in >>> pnv_machine_power8_class_init() to ensure that all PnvPHBs are created >>> with phb->version = 3. >>> >>> After all these changes we're still able to boot a powernv8 machine with >>> default settings. The real gain will come with user created PnvPHB >>> devices, coming up next. >>> >>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> --- >>> hw/pci-host/pnv_phb3.c | 29 ++++++++--------------------- >>> hw/ppc/pnv.c | 21 +++++++++++++++++---- >>> include/hw/pci-host/pnv_phb3.h | 5 ++++- >>> include/hw/ppc/pnv.h | 3 ++- >>> 4 files changed, 31 insertions(+), 27 deletions(-) >>> >>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c >>> index 60584e2aae..a39aa0e8c4 100644 >>> --- a/hw/pci-host/pnv_phb3.c >>> +++ b/hw/pci-host/pnv_phb3.c >>> @@ -11,6 +11,7 @@ >>> #include "qapi/visitor.h" >>> #include "qapi/error.h" >>> #include "hw/pci-host/pnv_phb3_regs.h" >>> +#include "hw/pci-host/pnv_phb.h" >>> #include "hw/pci-host/pnv_phb3.h" >>> #include "hw/pci/pcie_host.h" >>> #include "hw/pci/pcie_port.h" >>> @@ -26,7 +27,7 @@ >>> static PCIDevice *pnv_phb3_find_cfg_dev(PnvPHB3 *phb) >>> { >>> - PCIHostState *pci = PCI_HOST_BRIDGE(phb); >>> + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); >>> uint64_t addr = phb->regs[PHB_CONFIG_ADDRESS >> 3]; >>> uint8_t bus, devfn; >>> @@ -590,7 +591,7 @@ void pnv_phb3_reg_write(void *opaque, hwaddr off, uint64_t >>> val, unsigned size) >>> uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size) >>> { >>> PnvPHB3 *phb = opaque; >>> - PCIHostState *pci = PCI_HOST_BRIDGE(phb); >>> + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); >>> uint64_t val; >>> if ((off & 0xfffc) == PHB_CONFIG_DATA) { >>> @@ -1057,8 +1058,6 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp) >>> "phb3-regs", 0x1000); >>> pnv_phb3_bus_init(dev, phb); >>> - >>> - pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT); >>> } >>> void pnv_phb3_update_regions(PnvPHB3 *phb) >>> @@ -1083,38 +1082,26 @@ void pnv_phb3_update_regions(PnvPHB3 *phb) >>> pnv_phb3_check_all_m64s(phb); >>> } >>> -static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge, >>> - PCIBus *rootbus) >>> -{ >>> - PnvPHB3 *phb = PNV_PHB3(host_bridge); >>> - >>> - snprintf(phb->bus_path, sizeof(phb->bus_path), "00%02x:%02x", >>> - phb->chip_id, phb->phb_id); >>> - return phb->bus_path; >>> -} >>> - >>> static Property pnv_phb3_properties[] = { >>> DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0), >>> DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0), >>> DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *), >>> + DEFINE_PROP_LINK("phb-base", PnvPHB3, phb_base, TYPE_PNV_PHB, PnvPHB *), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> static void pnv_phb3_class_init(ObjectClass *klass, void *data) >>> { >>> - PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass); >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> - hc->root_bus_path = pnv_phb3_root_bus_path; >>> dc->realize = pnv_phb3_realize; >>> device_class_set_props(dc, pnv_phb3_properties); >>> - set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >>> dc->user_creatable = false; >>> } >>> static const TypeInfo pnv_phb3_type_info = { >>> .name = TYPE_PNV_PHB3, >>> - .parent = TYPE_PCIE_HOST_BRIDGE, >>> + .parent = TYPE_DEVICE, >>> .instance_size = sizeof(PnvPHB3), >>> .class_init = pnv_phb3_class_init, >>> .instance_init = pnv_phb3_instance_init, >>> @@ -1146,11 +1133,11 @@ 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; >>> + PnvPHB *phb = NULL; >>> Error *local_err = NULL; >>> - phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), >>> - TYPE_PNV_PHB3); >>> + phb = (PnvPHB *) object_dynamic_cast(OBJECT(bus->qbus.parent), >>> + TYPE_PNV_PHB); >> >> I realize that this has come from existing code, however these days there generally >> isn't a good reason for anything to access bus->qbus directly (and C casts without >> a QOM macro often need a closer look). I'm also not convinced by the use of >> object_dynamic_cast() here either. Could this be rewritten as something like: >> >> phb = PNV_PHB(PNV_PHB3_ROOT_PORT(dev)->phb_base); > > Just checked. At this moment we can't because we don't declare a PNV_PHB3_ROOT_PORT() > macro for this device. Yes, we're missing a OBJECT_DECLARE_SIMPLE_TYPE() for it. Same > thing for PnvPHB4RootPort. > > I can add the missing PNV_PHB3_ROOT_PORT() macro and do this change. Or I can make > a comment mentioning that this code can be improved but it's going to be removed > shortly and this will be amended appropriately in the pnv-phb-root-port code. > > > What do you think? I'm fine with both alternatives. I see there are some queries about the way these devices are modelled, so if that requires more than trivial changes it may make sense to integrate these changes into v3, particularly if it helps with type safety. Otherwise I'd be simply inclined to add these changes to the end of your existing series as a final tidy-up and post that as v3. > Thanks, > > > Daniel > >> >>> if (!phb) { >>> error_setg(errp, >>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >>> index aaf4d241c3..6cd0af9adf 100644 >>> --- a/hw/ppc/pnv.c >>> +++ b/hw/ppc/pnv.c >>> @@ -43,6 +43,7 @@ >>> #include "hw/ipmi/ipmi.h" >>> #include "target/ppc/mmu-hash64.h" >>> #include "hw/pci/msi.h" >>> +#include "hw/pci-host/pnv_phb.h" >>> #include "hw/ppc/xics.h" >>> #include "hw/qdev-properties.h" >>> @@ -654,7 +655,13 @@ static ISABus *pnv_isa_create(PnvChip *chip, Error **errp) >>> static PnvPHB3 *pnv_get_phb3_child(Object *child) >>> { >>> - return (PnvPHB3 *)object_dynamic_cast(child, TYPE_PNV_PHB3); >>> + PnvPHB *phb = (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB); >> >> And here, assuming child is of type TYPE_PNV_PHB then as it is already known at >> compile time so I think this could become just: >> >> PnvPHB *phb = PNV_PHB(child); >> >>> + if (!phb) { >>> + return NULL; >>> + } >>> + >>> + return (PnvPHB3 *)phb->backend; >>> } >>> static int pnv_chip_power8_pic_print_info_child(Object *child, void *opaque) >>> @@ -1160,7 +1167,7 @@ static void pnv_chip_power8_instance_init(Object *obj) >>> chip8->num_phbs = pcc->num_phbs; >>> for (i = 0; i < chip8->num_phbs; i++) { >>> - object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3); >>> + object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB); >>> } >>> } >>> @@ -1282,9 +1289,9 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error >>> **errp) >>> memory_region_add_subregion(get_system_memory(), PNV_HOMER_BASE(chip), >>> &chip8->homer.regs); >>> - /* PHB3 controllers */ >>> + /* PHB controllers */ >>> for (i = 0; i < chip8->num_phbs; i++) { >>> - PnvPHB3 *phb = &chip8->phbs[i]; >>> + PnvPHB *phb = &chip8->phbs[i]; >>> object_property_set_int(OBJECT(phb), "index", i, &error_fatal); >>> object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id, >>> @@ -1957,6 +1964,7 @@ static int pnv_ics_get_child(Object *child, void *opaque) >>> args->ics = ICS(&phb3->msis); >>> } >>> } >>> + >>> return args->ics ? 1 : 0; >>> } >>> @@ -2112,8 +2120,13 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, >>> void *data) >>> PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); >>> static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; >>> + static GlobalProperty phb_compat[] = { >>> + { TYPE_PNV_PHB, "version", "3" }, >>> + }; >>> + >>> mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; >>> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); >>> + compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat)); >>> xic->icp_get = pnv_icp_get; >>> xic->ics_get = pnv_ics_get; >>> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h >>> index 1375f18fc1..3b9ff1096a 100644 >>> --- a/include/hw/pci-host/pnv_phb3.h >>> +++ b/include/hw/pci-host/pnv_phb3.h >>> @@ -14,6 +14,7 @@ >>> #include "hw/pci/pcie_port.h" >>> #include "hw/ppc/xics.h" >>> #include "qom/object.h" >>> +#include "hw/pci-host/pnv_phb.h" >>> typedef struct PnvPHB3 PnvPHB3; >>> typedef struct PnvChip PnvChip; >>> @@ -127,7 +128,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB3, PNV_PHB3) >>> #define PCI_MMIO_TOTAL_SIZE (0x1ull << 60) >>> struct PnvPHB3 { >>> - PCIExpressHost parent_obj; >>> + DeviceState parent; >>> + >>> + PnvPHB *phb_base; >>> uint32_t chip_id; >>> uint32_t phb_id; >>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h >>> index 86cb7d7f97..4595db418e 100644 >>> --- a/include/hw/ppc/pnv.h >>> +++ b/include/hw/ppc/pnv.h >>> @@ -32,6 +32,7 @@ >>> #include "hw/ppc/pnv_core.h" >>> #include "hw/pci-host/pnv_phb3.h" >>> #include "hw/pci-host/pnv_phb4.h" >>> +#include "hw/pci-host/pnv_phb.h" >>> #include "qom/object.h" >>> #define TYPE_PNV_CHIP "pnv-chip" >>> @@ -80,7 +81,7 @@ struct Pnv8Chip { >>> PnvHomer homer; >>> #define PNV8_CHIP_PHB3_MAX 4 >>> - PnvPHB3 phbs[PNV8_CHIP_PHB3_MAX]; >>> + PnvPHB phbs[PNV8_CHIP_PHB3_MAX]; >>> uint32_t num_phbs; >>> XICSFabric *xics; ATB, Mark.
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index 60584e2aae..a39aa0e8c4 100644 --- a/hw/pci-host/pnv_phb3.c +++ b/hw/pci-host/pnv_phb3.c @@ -11,6 +11,7 @@ #include "qapi/visitor.h" #include "qapi/error.h" #include "hw/pci-host/pnv_phb3_regs.h" +#include "hw/pci-host/pnv_phb.h" #include "hw/pci-host/pnv_phb3.h" #include "hw/pci/pcie_host.h" #include "hw/pci/pcie_port.h" @@ -26,7 +27,7 @@ static PCIDevice *pnv_phb3_find_cfg_dev(PnvPHB3 *phb) { - PCIHostState *pci = PCI_HOST_BRIDGE(phb); + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); uint64_t addr = phb->regs[PHB_CONFIG_ADDRESS >> 3]; uint8_t bus, devfn; @@ -590,7 +591,7 @@ void pnv_phb3_reg_write(void *opaque, hwaddr off, uint64_t val, unsigned size) uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size) { PnvPHB3 *phb = opaque; - PCIHostState *pci = PCI_HOST_BRIDGE(phb); + PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base); uint64_t val; if ((off & 0xfffc) == PHB_CONFIG_DATA) { @@ -1057,8 +1058,6 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp) "phb3-regs", 0x1000); pnv_phb3_bus_init(dev, phb); - - pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT); } void pnv_phb3_update_regions(PnvPHB3 *phb) @@ -1083,38 +1082,26 @@ void pnv_phb3_update_regions(PnvPHB3 *phb) pnv_phb3_check_all_m64s(phb); } -static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge, - PCIBus *rootbus) -{ - PnvPHB3 *phb = PNV_PHB3(host_bridge); - - snprintf(phb->bus_path, sizeof(phb->bus_path), "00%02x:%02x", - phb->chip_id, phb->phb_id); - return phb->bus_path; -} - static Property pnv_phb3_properties[] = { DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0), DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0), DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *), + DEFINE_PROP_LINK("phb-base", PnvPHB3, phb_base, TYPE_PNV_PHB, PnvPHB *), DEFINE_PROP_END_OF_LIST(), }; static void pnv_phb3_class_init(ObjectClass *klass, void *data) { - PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass); DeviceClass *dc = DEVICE_CLASS(klass); - hc->root_bus_path = pnv_phb3_root_bus_path; dc->realize = pnv_phb3_realize; device_class_set_props(dc, pnv_phb3_properties); - set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->user_creatable = false; } static const TypeInfo pnv_phb3_type_info = { .name = TYPE_PNV_PHB3, - .parent = TYPE_PCIE_HOST_BRIDGE, + .parent = TYPE_DEVICE, .instance_size = sizeof(PnvPHB3), .class_init = pnv_phb3_class_init, .instance_init = pnv_phb3_instance_init, @@ -1146,11 +1133,11 @@ 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; + PnvPHB *phb = NULL; Error *local_err = NULL; - phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent), - TYPE_PNV_PHB3); + phb = (PnvPHB *) object_dynamic_cast(OBJECT(bus->qbus.parent), + TYPE_PNV_PHB); if (!phb) { error_setg(errp, diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index aaf4d241c3..6cd0af9adf 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -43,6 +43,7 @@ #include "hw/ipmi/ipmi.h" #include "target/ppc/mmu-hash64.h" #include "hw/pci/msi.h" +#include "hw/pci-host/pnv_phb.h" #include "hw/ppc/xics.h" #include "hw/qdev-properties.h" @@ -654,7 +655,13 @@ static ISABus *pnv_isa_create(PnvChip *chip, Error **errp) static PnvPHB3 *pnv_get_phb3_child(Object *child) { - return (PnvPHB3 *)object_dynamic_cast(child, TYPE_PNV_PHB3); + PnvPHB *phb = (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB); + + if (!phb) { + return NULL; + } + + return (PnvPHB3 *)phb->backend; } static int pnv_chip_power8_pic_print_info_child(Object *child, void *opaque) @@ -1160,7 +1167,7 @@ static void pnv_chip_power8_instance_init(Object *obj) chip8->num_phbs = pcc->num_phbs; for (i = 0; i < chip8->num_phbs; i++) { - object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3); + object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB); } } @@ -1282,9 +1289,9 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp) memory_region_add_subregion(get_system_memory(), PNV_HOMER_BASE(chip), &chip8->homer.regs); - /* PHB3 controllers */ + /* PHB controllers */ for (i = 0; i < chip8->num_phbs; i++) { - PnvPHB3 *phb = &chip8->phbs[i]; + PnvPHB *phb = &chip8->phbs[i]; object_property_set_int(OBJECT(phb), "index", i, &error_fatal); object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id, @@ -1957,6 +1964,7 @@ static int pnv_ics_get_child(Object *child, void *opaque) args->ics = ICS(&phb3->msis); } } + return args->ics ? 1 : 0; } @@ -2112,8 +2120,13 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; + static GlobalProperty phb_compat[] = { + { TYPE_PNV_PHB, "version", "3" }, + }; + mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); + compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat)); xic->icp_get = pnv_icp_get; xic->ics_get = pnv_ics_get; diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h index 1375f18fc1..3b9ff1096a 100644 --- a/include/hw/pci-host/pnv_phb3.h +++ b/include/hw/pci-host/pnv_phb3.h @@ -14,6 +14,7 @@ #include "hw/pci/pcie_port.h" #include "hw/ppc/xics.h" #include "qom/object.h" +#include "hw/pci-host/pnv_phb.h" typedef struct PnvPHB3 PnvPHB3; typedef struct PnvChip PnvChip; @@ -127,7 +128,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB3, PNV_PHB3) #define PCI_MMIO_TOTAL_SIZE (0x1ull << 60) struct PnvPHB3 { - PCIExpressHost parent_obj; + DeviceState parent; + + PnvPHB *phb_base; uint32_t chip_id; uint32_t phb_id; diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h index 86cb7d7f97..4595db418e 100644 --- a/include/hw/ppc/pnv.h +++ b/include/hw/ppc/pnv.h @@ -32,6 +32,7 @@ #include "hw/ppc/pnv_core.h" #include "hw/pci-host/pnv_phb3.h" #include "hw/pci-host/pnv_phb4.h" +#include "hw/pci-host/pnv_phb.h" #include "qom/object.h" #define TYPE_PNV_CHIP "pnv-chip" @@ -80,7 +81,7 @@ struct Pnv8Chip { PnvHomer homer; #define PNV8_CHIP_PHB3_MAX 4 - PnvPHB3 phbs[PNV8_CHIP_PHB3_MAX]; + PnvPHB phbs[PNV8_CHIP_PHB3_MAX]; uint32_t num_phbs; XICSFabric *xics;
We need a handful of changes that needs to be done in a single swoop to turn PnvPHB3 into a PnvPHB backend. In the PnvPHB3, since the PnvPHB device implements PCIExpressHost and will hold the PCI bus, change PnvPHB3 parent to TYPE_DEVICE. There are a couple of instances in pnv_phb3.c that needs to access the PCI bus, so a phb_base pointer is added to allow access to the parent PnvPHB. The PnvPHB3 root port will now be connected to a PnvPHB object. In pnv.c, the powernv8 machine chip8 will now hold an array of PnvPHB objects. pnv_get_phb3_child() needs to be adapted to return the PnvPHB3 backend from the PnvPHB child. A global property is added in pnv_machine_power8_class_init() to ensure that all PnvPHBs are created with phb->version = 3. After all these changes we're still able to boot a powernv8 machine with default settings. The real gain will come with user created PnvPHB devices, coming up next. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb3.c | 29 ++++++++--------------------- hw/ppc/pnv.c | 21 +++++++++++++++++---- include/hw/pci-host/pnv_phb3.h | 5 ++++- include/hw/ppc/pnv.h | 3 ++- 4 files changed, 31 insertions(+), 27 deletions(-)