| Message ID | 20220803134459.2633902-7-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable pnv-phb user created devices | expand |
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c > index 077f391d59..953c384bf6 100644 > --- a/hw/pci-host/pnv_phb.c > +++ b/hw/pci-host/pnv_phb.c > @@ -17,6 +17,7 @@ > #include "hw/ppc/pnv.h" > #include "hw/qdev-properties.h" > #include "qom/object.h" > +#include "sysemu/sysemu.h" > > > /* > @@ -171,6 +172,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp) > pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend)); > } > > + if (phb->version == 3 && !defaults_enabled()) { > + return; > + } > + > pnv_phb_attach_root_port(pci); > } So we skip the call to pnv_phb_attach_root_port() for the user created PHBs. I seem to remember that when the user creates a PHB on the command line, they also need to create the root port and we can't just create one by default with the PHB. It seems heavy when just using the qemu CLI but that was a requirement from libvirt. Did I get that right? In any case, I can see a problem. pnv_phb_attach_root_port() creates the root port object, so if the above is correct, we want to skip that. But it also creates the PHB->root port relationship in QOM. That we don't do anywhere for the user-created case. Calling qemu with "-nodefaults -device pnv-phb -device pnv-phb-root-port", here is what I see in QOM, which shows the wrong parenting of the root port: /peripheral-anon (container) /device[1] (pnv-phb-root-port) /bus master container[0] (memory-region) /bus master[0] (memory-region) /pci_bridge_io[0] (memory-region) /pci_bridge_io[1] (memory-region) /pci_bridge_mem[0] (memory-region) /pci_bridge_pci[0] (memory-region) /pci_bridge_pref_mem[0] (memory-region) /pci_bridge_vga_io_hi[0] (memory-region) /pci_bridge_vga_io_lo[0] (memory-region) /pci_bridge_vga_mem[0] (memory-region) /pcie.0 (PCIE) With the full series, same problem on P8/P9/P10 Fred
On 8/5/22 07:50, Frederic Barrat wrote: > > >> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c >> index 077f391d59..953c384bf6 100644 >> --- a/hw/pci-host/pnv_phb.c >> +++ b/hw/pci-host/pnv_phb.c >> @@ -17,6 +17,7 @@ >> #include "hw/ppc/pnv.h" >> #include "hw/qdev-properties.h" >> #include "qom/object.h" >> +#include "sysemu/sysemu.h" >> /* >> @@ -171,6 +172,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp) >> pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend)); >> } >> + if (phb->version == 3 && !defaults_enabled()) { >> + return; >> + } >> + >> pnv_phb_attach_root_port(pci); >> } > > > So we skip the call to pnv_phb_attach_root_port() for the user created PHBs. > > I seem to remember that when the user creates a PHB on the command line, they also need to create the root port and we can't just create one by default with the PHB. It seems heavy when just using the qemu CLI but that was a requirement from libvirt. Did I get that right? libvirt requires all pci devices to be declared in the XML. If we create the root ports automatically by just adding PHBs I would need to reflect that in the domain XML by either automatically add root ports with each PHB, which is easier said than done, or tell the user to do so. libvirt also allows for custom naming of the buses, e.g the PHB can be named 'phb-root0' and its root port 'my-rootport.1'. There's no way we can name root ports buses by adding them automatically with each PHB like we do for default PHBs. > > In any case, I can see a problem. pnv_phb_attach_root_port() creates the root port object, so if the above is correct, we want to skip that. But it also creates the PHB->root port relationship in QOM. That we don't do anywhere for the user-created case. > > Calling qemu with "-nodefaults -device pnv-phb -device pnv-phb-root-port", here is what I see in QOM, which shows the wrong parenting of the root port: > /peripheral-anon (container) > /device[1] (pnv-phb-root-port) > /bus master container[0] (memory-region) > /bus master[0] (memory-region) > /pci_bridge_io[0] (memory-region) > /pci_bridge_io[1] (memory-region) > /pci_bridge_mem[0] (memory-region) > /pci_bridge_pci[0] (memory-region) > /pci_bridge_pref_mem[0] (memory-region) > /pci_bridge_vga_io_hi[0] (memory-region) > /pci_bridge_vga_io_lo[0] (memory-region) > /pci_bridge_vga_mem[0] (memory-region) > /pcie.0 (PCIE) > > > With the full series, same problem on P8/P9/P10 I forgot to mention it in the cover letter. This is intended. The reason is that we can't access the PHB from the root-port due to QOM best practices, i.e. we can't do qdev_get_parent_bus()->parent to parent the root port with the PHB. The reason why we're able to do that for default devices is because we have access to the PHB regardless of QOM when creating the root port. If we want QOM consistency between user created devices and default devices, one alternative is to make the root-port a child of the root bus. That can be achievable in both cases. Thanks, Daniel > > Fred > > > > >
On 8/8/22 13:50, Daniel Henrique Barboza wrote: > > > On 8/5/22 07:50, Frederic Barrat wrote: >> >> >>> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c >>> index 077f391d59..953c384bf6 100644 >>> --- a/hw/pci-host/pnv_phb.c >>> +++ b/hw/pci-host/pnv_phb.c >>> @@ -17,6 +17,7 @@ >>> #include "hw/ppc/pnv.h" >>> #include "hw/qdev-properties.h" >>> #include "qom/object.h" >>> +#include "sysemu/sysemu.h" >>> /* >>> @@ -171,6 +172,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp) >>> pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend)); >>> } >>> + if (phb->version == 3 && !defaults_enabled()) { >>> + return; >>> + } >>> + >>> pnv_phb_attach_root_port(pci); >>> } >> >> >> So we skip the call to pnv_phb_attach_root_port() for the user created PHBs. >> >> I seem to remember that when the user creates a PHB on the command line, they also need to create the root port and we can't just create one by default with the PHB. It seems heavy when just using the qemu CLI but that was a requirement from libvirt. Did I get that right? > > libvirt requires all pci devices to be declared in the XML. If we create the > root ports automatically by just adding PHBs I would need to reflect that > in the domain XML by either automatically add root ports with each PHB, which > is easier said than done, or tell the user to do so. > > libvirt also allows for custom naming of the buses, e.g the PHB can be named > 'phb-root0' and its root port 'my-rootport.1'. There's no way we can name > root ports buses by adding them automatically with each PHB like we do > for default PHBs. > > >> >> In any case, I can see a problem. pnv_phb_attach_root_port() creates the root port object, so if the above is correct, we want to skip that. But it also creates the PHB->root port relationship in QOM. That we don't do anywhere for the user-created case. >> >> Calling qemu with "-nodefaults -device pnv-phb -device pnv-phb-root-port", here is what I see in QOM, which shows the wrong parenting of the root port: >> /peripheral-anon (container) >> /device[1] (pnv-phb-root-port) >> /bus master container[0] (memory-region) >> /bus master[0] (memory-region) >> /pci_bridge_io[0] (memory-region) >> /pci_bridge_io[1] (memory-region) >> /pci_bridge_mem[0] (memory-region) >> /pci_bridge_pci[0] (memory-region) >> /pci_bridge_pref_mem[0] (memory-region) >> /pci_bridge_vga_io_hi[0] (memory-region) >> /pci_bridge_vga_io_lo[0] (memory-region) >> /pci_bridge_vga_mem[0] (memory-region) >> /pcie.0 (PCIE) >> >> >> With the full series, same problem on P8/P9/P10 > > I forgot to mention it in the cover letter. This is intended. The reason > is that we can't access the PHB from the root-port due to QOM best > practices, i.e. we can't do qdev_get_parent_bus()->parent to parent > the root port with the PHB. The reason why we're able to do that for > default devices is because we have access to the PHB regardless of QOM > when creating the root port. This is not correct. The default phb is parented by the root-bus, not the PHB. E.g: /pnv-phb4-root.9 (pnv-phb4-root) /pnv-phb-root-port[3] (pnv-phb-root-port) /bus master container[0] (memory-region) /bus master[0] (memory-region) (...) Both default and user created cases have access to the phb-root-bus. The reasoning behind the current QOM hierarchy is the existing design in place for what it is called "dynamically created sysbus devices", which means all the devices that are created via command line. If the device has an 'id' it'll be inserted in a 'peripheral' container, if no 'id' is provided the device goes into a 'peripheral-anon' container. I managed to amend the root_port_realize() function to place the root-port in the same place as the default root port appears. What I need to do now before re-sending is verify whether I broke something else during the process :) Thanks, Daniel > > > If we want QOM consistency between user created devices and default > devices, one alternative is to make the root-port a child of the > root bus. That can be achievable in both cases. > > > Thanks, > > > Daniel > > >> >> Fred >> >> >> >> >>
diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c index 077f391d59..953c384bf6 100644 --- a/hw/pci-host/pnv_phb.c +++ b/hw/pci-host/pnv_phb.c @@ -17,6 +17,7 @@ #include "hw/ppc/pnv.h" #include "hw/qdev-properties.h" #include "qom/object.h" +#include "sysemu/sysemu.h" /* @@ -171,6 +172,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp) pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend)); } + if (phb->version == 3 && !defaults_enabled()) { + return; + } + pnv_phb_attach_root_port(pci); } @@ -206,7 +211,7 @@ static void pnv_phb_class_init(ObjectClass *klass, void *data) dc->realize = pnv_phb_realize; device_class_set_props(dc, pnv_phb_properties); set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); - dc->user_creatable = false; + dc->user_creatable = true; } static void pnv_phb_root_port_reset(DeviceState *dev) @@ -297,7 +302,7 @@ static void pnv_phb_root_port_class_init(ObjectClass *klass, void *data) device_class_set_parent_reset(dc, pnv_phb_root_port_reset, &rpc->parent_reset); dc->reset = &pnv_phb_root_port_reset; - dc->user_creatable = false; + dc->user_creatable = true; k->vendor_id = PCI_VENDOR_ID_IBM; /* device_id will be written during realize() */ diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 44066ffb8c..0d3a88578b 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1151,6 +1151,10 @@ static void pnv_chip_power8_instance_init(Object *obj) object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER); + if (!defaults_enabled()) { + return; + } + chip8->num_phbs = pcc->num_phbs; for (i = 0; i < chip8->num_phbs; i++) { @@ -2103,6 +2107,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) pmc->compat = compat; pmc->compat_size = sizeof(compat); + + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB); } static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
The bulk of the work was already done by previous patches. Use defaults_enabled() to determine whether we need to create the default devices or not. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb.c | 9 +++++++-- hw/ppc/pnv.c | 6 ++++++ 2 files changed, 13 insertions(+), 2 deletions(-)