| Message ID | 20220507190624.507419-1-danielhb413@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | powernv: introduce pnv-phb unified devices | expand |
On 07/05/2022 20:06, Daniel Henrique Barboza wrote: > Hi, > > Since the 7.0.0 release cycle we have a desire to use the powernv > emulation with libvirt. To do that we need to enable user creatable > pnv-phb devices to allow more user customization an to avoid spamming > multiple default devices in the domain XML. In the middle of the > previous cycle we experimented with user created > pnv-phb3/pnv-phb3-root-port/pnv-phb4/pnv-phb4-root-port/pnv-phb5. The > end result, although functional, is that the user needs to deal with a > lot of versioned devices that, from the user perspective, does the same > thing. In a way we outsourced the implementation details of the PHBs > (e.g. pnv8 uses phb3, pnv9 uses phb4) to the command line. Having more > devices also puts an extra burden in the libvirt support we want to > have. > > To solve this, Cedric and Frederic gave the idea of adding a common > virtual pnv-phb device that the user can add in the command line, and > QEMU handles the details internally. Unfortunatelly this idea turned out > to be way harder than anticipated. Between creating a device that would > just forward the callbacks to the existing devices internally, creating > a PnvPHB device with the minimal attributes and making the other devices > inherit from it, and making an 'abstract' device that could be casted > for both phb3 and phb4 PHBs, This bit sounds all good... > all sorts of very obscure problems occured: > PHBs not being detected, interrupts not being delivered and memory > regions not being able to read/write registers. My initial impression is > that there are assumptions made both in ppc/pnv and hw/pci-host that > requires the memory region and the bus being in the same device. Even > if we somehow figure all this out, the resulting code is hacky and > annoying to maitain. But this seems really surprising since there are many examples of similar QOM patterns within the codebase, and in my experience they work really well. Do you have a particular example that you can share to demonstrate why the result of the QOM mapping is making things more difficult? > This brings us to this series. The cleaner way I found to accomplish > what we want to do is to create real, unified pnv-phb/phb-phb-root-port > devices, and get rid of all the versioned ones. This is done by > merging all the PHB3/PHB4 attributes in unified devices. pnv_phb3* and pnv_phb4* > files end up using the same pnv-phb and phb-phb-root-port unified devices, > with the difference that pnv_phb3* only cares about version 3 attributes > and pnv_phb4* only cares about PHB4 attributes. Introducing new PHB > versions in the future will be a matter of adding any non-existent > attributes in the unified pnv-phb* devices and using them in separated > pnv_phbN* files. > > The pnv-phb implementation per se is just a router for either phb3 or > phb4 logic, done in init() and realize() time, after we check which powernv > machine we're running. If running with powernv8 we redirect control to > pnv_phb3_realize(), otherwise we redirect to pnv_phb4_realize(). The > unified device does not do anything per se other than handling control > to the right version. > > After this series, this same powernv8 command line that boots a powernv8 > machine with some phbs and root ports and with network: > > ./qemu-system-ppc64 -m 4G \ > -machine powernv8 -smp 2,sockets=2,cores=1,threads=1 \ > -accel tcg,thread=multi -bios skiboot.lid \ > -kernel vmlinux -initrd buildroot.rootfs.cpio -append 'console=hvc0 ro xmon=on' \ > -nodefaults -serial mon:stdio -nographic \ > -device pnv-phb,chip-id=0,index=0,id=pcie.0 \ > -device pnv-phb,chip-id=0,index=1,id=pcie.1 \ > -device pnv-phb,chip-id=1,index=2,id=pcie.2 \ > -device pnv-phb-root-port,id=root0,bus=pcie.2 \ > -device pnv-phb-root-port,id=root1,bus=pcie.1 \ > -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \ > -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \ > -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \ > -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \ > -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \ > -device nec-usb-xhci,bus=bridge1,addr=0x2 > > > Can be used to boot powernv9 and powernv10 machines with the same attributes > just by changing the machine type. > > > Daniel Henrique Barboza (17): > ppc/pnv: rename PnvPHB3.ioda* to PnvPHB3.ioda2* > ppc/pnv: rename PnvPHB3.regs[] to PnvPHB3.regs3[] > ppc/pnv: rename PnvPHB3.dma_spaces to PnvPHB3.v3_dma_spaces > ppc/pnv: add unified pnv-phb header > ppc/pnv: add pnv-phb device > ppc/pnv: remove PnvPHB3 > ppc/pnv: user created pnv-phb for powernv8 > ppc/pnv: remove PnvPHB4 > ppc/pnv: user creatable pnv-phb for powernv9 > ppc/pnv: use PnvPHB.version > ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs > ppc/pnv: user creatable pnv-phb for powernv10 > ppc/pnv: add pnv_phb_get_current_machine() > ppc/pnv: add pnv-phb-root-port device > ppc/pnv: remove pnv-phb3-root-port > ppc/pnv: remove pnv-phb4-root-port > ppc/pnv: remove pecc->rp_model > > hw/pci-host/meson.build | 3 +- > hw/pci-host/pnv_phb.c | 258 ++++++++++++++++++++++++++++ > hw/pci-host/pnv_phb3.c | 256 +++++++++++----------------- > hw/pci-host/pnv_phb3_msi.c | 12 +- > hw/pci-host/pnv_phb3_pbcq.c | 8 +- > hw/pci-host/pnv_phb4.c | 298 ++++++++++++--------------------- > hw/pci-host/pnv_phb4_pec.c | 14 +- > hw/ppc/pnv.c | 41 ++++- > include/hw/pci-host/pnv_phb.h | 224 +++++++++++++++++++++++++ > include/hw/pci-host/pnv_phb3.h | 118 +------------ > include/hw/pci-host/pnv_phb4.h | 108 ++---------- > include/hw/ppc/pnv.h | 3 +- > 12 files changed, 757 insertions(+), 586 deletions(-) > create mode 100644 hw/pci-host/pnv_phb.c > create mode 100644 include/hw/pci-host/pnv_phb.h I'm completely on-board with having a proxy-like PHB device that maps to the correct underlying PHB version, but to me it feels like combining multiple separate devices into a single one is going to make things more complicated to maintain in the long term. ATB, Mark.
On 5/9/22 18:17, Mark Cave-Ayland wrote: > On 07/05/2022 20:06, Daniel Henrique Barboza wrote: > >> Hi, >> >> Since the 7.0.0 release cycle we have a desire to use the powernv >> emulation with libvirt. To do that we need to enable user creatable >> pnv-phb devices to allow more user customization an to avoid spamming >> multiple default devices in the domain XML. In the middle of the >> previous cycle we experimented with user created >> pnv-phb3/pnv-phb3-root-port/pnv-phb4/pnv-phb4-root-port/pnv-phb5. The >> end result, although functional, is that the user needs to deal with a >> lot of versioned devices that, from the user perspective, does the same >> thing. In a way we outsourced the implementation details of the PHBs >> (e.g. pnv8 uses phb3, pnv9 uses phb4) to the command line. Having more >> devices also puts an extra burden in the libvirt support we want to >> have. >> >> To solve this, Cedric and Frederic gave the idea of adding a common >> virtual pnv-phb device that the user can add in the command line, and >> QEMU handles the details internally. Unfortunatelly this idea turned out >> to be way harder than anticipated. Between creating a device that would >> just forward the callbacks to the existing devices internally, creating >> a PnvPHB device with the minimal attributes and making the other devices >> inherit from it, and making an 'abstract' device that could be casted >> for both phb3 and phb4 PHBs, > > This bit sounds all good... > >> all sorts of very obscure problems occured: >> PHBs not being detected, interrupts not being delivered and memory >> regions not being able to read/write registers. My initial impression is >> that there are assumptions made both in ppc/pnv and hw/pci-host that >> requires the memory region and the bus being in the same device. Even >> if we somehow figure all this out, the resulting code is hacky and >> annoying to maitain. > > But this seems really surprising since there are many examples of similar QOM patterns within the codebase, and in my experience they work really well. Do you have a particular example that you can share to demonstrate why the result of the QOM mapping is making things more difficult? It's not so much about the OOM getting in the way, but rather myself not being able to use QOM in a way to get what I want. There is a good probability that QOM is able to do it. Talking about the 2 PHBs pnv-phb3 (powernv8 only) and pnv-phb4 (powernv9 only), what we want is a way to let the user instantiate both using an alias, or an abstract object if you will, called "pnv-phb". This alias/abstraction would instantiate either a pnv-phb3 or a pnv-phb4 depending on the actual machine running. QOM has the "interface" concept that is used internally to make the device behave like the interface describes it, but I wasn't able to expose this kind of object to the user. It also has the "abstract" concept that, by the documentation itself, isn't supposed to be user created. Eventually I gave up this idea, accepting that only real devices can be exposed to the user (please correct me if I'm wrong). After that I tried to create a pnv-phb object that contains the common attributes of both pnv-phb3 and pnv-phb4. This object would be the parent of phb3 and phb4, and during realize time creating a pnv-phb object would create a pnv-phb3/4 and we go from there. This attempt went far after a few tweaks but then I started to hit the problems I mentioned above: some strange behaviors with interrupts, PHBs not appearing and so on. I got a little farther by moving all the memory regions from phb3/phb4 to the parent phb object and I got up to the guest login, but without network. Since moving the memory regions in the same object as the pci root bus got me farther I concluded that there might some relation/assumption made somewhere that I was breaking before. After all that I got more than halfway of what I ended up doing. I decided to unify phb3/phb4 into a single device, renamed their attributes that has different meanings between v3 and v4 code, and I ended up with this series. > >> This brings us to this series. The cleaner way I found to accomplish >> what we want to do is to create real, unified pnv-phb/phb-phb-root-port >> devices, and get rid of all the versioned ones. This is done by >> merging all the PHB3/PHB4 attributes in unified devices. pnv_phb3* and pnv_phb4* >> files end up using the same pnv-phb and phb-phb-root-port unified devices, >> with the difference that pnv_phb3* only cares about version 3 attributes >> and pnv_phb4* only cares about PHB4 attributes. Introducing new PHB >> versions in the future will be a matter of adding any non-existent >> attributes in the unified pnv-phb* devices and using them in separated >> pnv_phbN* files. >> >> The pnv-phb implementation per se is just a router for either phb3 or >> phb4 logic, done in init() and realize() time, after we check which powernv >> machine we're running. If running with powernv8 we redirect control to >> pnv_phb3_realize(), otherwise we redirect to pnv_phb4_realize(). The >> unified device does not do anything per se other than handling control >> to the right version. >> >> After this series, this same powernv8 command line that boots a powernv8 >> machine with some phbs and root ports and with network: >> >> ./qemu-system-ppc64 -m 4G \ >> -machine powernv8 -smp 2,sockets=2,cores=1,threads=1 \ >> -accel tcg,thread=multi -bios skiboot.lid \ >> -kernel vmlinux -initrd buildroot.rootfs.cpio -append 'console=hvc0 ro xmon=on' \ >> -nodefaults -serial mon:stdio -nographic \ >> -device pnv-phb,chip-id=0,index=0,id=pcie.0 \ >> -device pnv-phb,chip-id=0,index=1,id=pcie.1 \ >> -device pnv-phb,chip-id=1,index=2,id=pcie.2 \ >> -device pnv-phb-root-port,id=root0,bus=pcie.2 \ >> -device pnv-phb-root-port,id=root1,bus=pcie.1 \ >> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \ >> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \ >> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \ >> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \ >> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \ >> -device nec-usb-xhci,bus=bridge1,addr=0x2 >> >> >> Can be used to boot powernv9 and powernv10 machines with the same attributes >> just by changing the machine type. >> >> >> Daniel Henrique Barboza (17): >> ppc/pnv: rename PnvPHB3.ioda* to PnvPHB3.ioda2* >> ppc/pnv: rename PnvPHB3.regs[] to PnvPHB3.regs3[] >> ppc/pnv: rename PnvPHB3.dma_spaces to PnvPHB3.v3_dma_spaces >> ppc/pnv: add unified pnv-phb header >> ppc/pnv: add pnv-phb device >> ppc/pnv: remove PnvPHB3 >> ppc/pnv: user created pnv-phb for powernv8 >> ppc/pnv: remove PnvPHB4 >> ppc/pnv: user creatable pnv-phb for powernv9 >> ppc/pnv: use PnvPHB.version >> ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs >> ppc/pnv: user creatable pnv-phb for powernv10 >> ppc/pnv: add pnv_phb_get_current_machine() >> ppc/pnv: add pnv-phb-root-port device >> ppc/pnv: remove pnv-phb3-root-port >> ppc/pnv: remove pnv-phb4-root-port >> ppc/pnv: remove pecc->rp_model >> >> hw/pci-host/meson.build | 3 +- >> hw/pci-host/pnv_phb.c | 258 ++++++++++++++++++++++++++++ >> hw/pci-host/pnv_phb3.c | 256 +++++++++++----------------- >> hw/pci-host/pnv_phb3_msi.c | 12 +- >> hw/pci-host/pnv_phb3_pbcq.c | 8 +- >> hw/pci-host/pnv_phb4.c | 298 ++++++++++++--------------------- >> hw/pci-host/pnv_phb4_pec.c | 14 +- >> hw/ppc/pnv.c | 41 ++++- >> include/hw/pci-host/pnv_phb.h | 224 +++++++++++++++++++++++++ >> include/hw/pci-host/pnv_phb3.h | 118 +------------ >> include/hw/pci-host/pnv_phb4.h | 108 ++---------- >> include/hw/ppc/pnv.h | 3 +- >> 12 files changed, 757 insertions(+), 586 deletions(-) >> create mode 100644 hw/pci-host/pnv_phb.c >> create mode 100644 include/hw/pci-host/pnv_phb.h > > I'm completely on-board with having a proxy-like PHB device that maps to the correct underlying PHB version, but to me it feels like combining multiple separate devices into a single one is going to make things more complicated to maintain in the long term. I'm all up for a "virtual pnv-phb" device that can be used together with the existing versioned phbs. I wasn't able to pull that off. Thanks, Daniel > > > ATB, > > Mark.
On 09/05/2022 23:30, Daniel Henrique Barboza wrote: > On 5/9/22 18:17, Mark Cave-Ayland wrote: >> On 07/05/2022 20:06, Daniel Henrique Barboza wrote: >> >>> Hi, >>> >>> Since the 7.0.0 release cycle we have a desire to use the powernv >>> emulation with libvirt. To do that we need to enable user creatable >>> pnv-phb devices to allow more user customization an to avoid spamming >>> multiple default devices in the domain XML. In the middle of the >>> previous cycle we experimented with user created >>> pnv-phb3/pnv-phb3-root-port/pnv-phb4/pnv-phb4-root-port/pnv-phb5. The >>> end result, although functional, is that the user needs to deal with a >>> lot of versioned devices that, from the user perspective, does the same >>> thing. In a way we outsourced the implementation details of the PHBs >>> (e.g. pnv8 uses phb3, pnv9 uses phb4) to the command line. Having more >>> devices also puts an extra burden in the libvirt support we want to >>> have. >>> >>> To solve this, Cedric and Frederic gave the idea of adding a common >>> virtual pnv-phb device that the user can add in the command line, and >>> QEMU handles the details internally. Unfortunatelly this idea turned out >>> to be way harder than anticipated. Between creating a device that would >>> just forward the callbacks to the existing devices internally, creating >>> a PnvPHB device with the minimal attributes and making the other devices >>> inherit from it, and making an 'abstract' device that could be casted >>> for both phb3 and phb4 PHBs, >> >> This bit sounds all good... >> >>> all sorts of very obscure problems occured: >>> PHBs not being detected, interrupts not being delivered and memory >>> regions not being able to read/write registers. My initial impression is >>> that there are assumptions made both in ppc/pnv and hw/pci-host that >>> requires the memory region and the bus being in the same device. Even >>> if we somehow figure all this out, the resulting code is hacky and >>> annoying to maitain. >> >> But this seems really surprising since there are many examples of similar QOM >> patterns within the codebase, and in my experience they work really well. Do you >> have a particular example that you can share to demonstrate why the result of the >> QOM mapping is making things more difficult? > > > It's not so much about the OOM getting in the way, but rather myself not being > able to use QOM in a way to get what I want. There is a good probability that > QOM is able to do it. > > Talking about the 2 PHBs pnv-phb3 (powernv8 only) and pnv-phb4 (powernv9 only), > what we want is a way to let the user instantiate both using an alias, or > an abstract object if you will, called "pnv-phb". This alias/abstraction would > instantiate either a pnv-phb3 or a pnv-phb4 depending on the actual machine > running. > > QOM has the "interface" concept that is used internally to make the device behave > like the interface describes it, but I wasn't able to expose this kind of object > to the user. It also has the "abstract" concept that, by the documentation itself, > isn't supposed to be user created. Eventually I gave up this idea, accepting that > only real devices can be exposed to the user (please correct me if I'm wrong). Sorry, I should have clarified this a bit more: introducing an abstract type is a separate task from adding your proxy device type, but I think will definitely help maintaining the individual versions. Certainly it will make it easier to see which fields are in use by specific versions, and it also gives you a QOM cast for the superclass of all PHB devices to help with type checking. > After that I tried to create a pnv-phb object that contains the common attributes > of both pnv-phb3 and pnv-phb4. This object would be the parent of phb3 and phb4, > and during realize time creating a pnv-phb object would create a pnv-phb3/4 and > we go from there. This attempt went far after a few tweaks but then I started > to hit the problems I mentioned above: some strange behaviors with interrupts, > PHBs not appearing and so on. I got a little farther by moving all the memory > regions from phb3/phb4 to the parent phb object and I got up to the guest login, > but without network. Since moving the memory regions in the same object as the > pci root bus got me farther I concluded that there might some relation/assumption > made somewhere that I was breaking before. That sounds really odd. The normal reasons for this in my experience are i) forgetting to update the class/instance size in the class_init function (an --enable-sanitizers build will catch this) and ii) not propagating device reset/realize functions to a parent device leading to uninitialised state. > After all that I got more than halfway of what I ended up doing. I decided to > unify phb3/phb4 into a single device, renamed their attributes that has different > meanings between v3 and v4 code, and I ended up with this series. As a rough outline for a pnv-phb device, I'd aim for creating a proxy for the underlying device rather than manually invoking the QOM instance and qdev-related functions: struct PnvPHB { .... uint32_t version; Object *phb_dev; /* Could be PHBCommonBase if it exists */ }; DECLARE_SIMPLE_OBJECT_TYPE(...) ... ... static Property pnv_phb_properties[] = { DEFINE_PROP_UINT32("version", PnvPHB, version, 0), DEFINE_PROP_END_OF_LIST(), }; static void pnv_phb_realize(DeviceState *dev, Error **errp) { PnvPHB *pnv_phb = PNV_PHB(dev); g_autofree char *phb_typename; if (!pnv_phb->version) { error_setg("version not specified", errp); return; } switch (pnv_phb->version) { case 3: phb_typename = g_strdup(TYPE_PNV_PHB3); break; case 4: phb_typename = g_strdup(TYPE_PNV_PHB4); break; default: g_assert_unreached(); } pnv_phb->phb_dev = object_new(phb_typename); object_property_add_child(OBJECT(dev), "phb-device", pnv_phb->phb_dev); if (!qdev_realize(DEVICE(pnv_phb->phb_dev), errp)) { return; } /* Passthrough child device properties to the proxy device */ qdev_alias_all_properties(dev, OBJECT(pnv_phb->phb_dev)); } Finally you can set the pnv-phb version on a per-machine basis by adding the version to the machine compat_props: static GlobalProperty compat[] = { { TYPE_PHB_PNV, "version", 3}, }; >>> This brings us to this series. The cleaner way I found to accomplish >>> what we want to do is to create real, unified pnv-phb/phb-phb-root-port >>> devices, and get rid of all the versioned ones. This is done by >>> merging all the PHB3/PHB4 attributes in unified devices. pnv_phb3* and pnv_phb4* >>> files end up using the same pnv-phb and phb-phb-root-port unified devices, >>> with the difference that pnv_phb3* only cares about version 3 attributes >>> and pnv_phb4* only cares about PHB4 attributes. Introducing new PHB >>> versions in the future will be a matter of adding any non-existent >>> attributes in the unified pnv-phb* devices and using them in separated >>> pnv_phbN* files. >>> >>> The pnv-phb implementation per se is just a router for either phb3 or >>> phb4 logic, done in init() and realize() time, after we check which powernv >>> machine we're running. If running with powernv8 we redirect control to >>> pnv_phb3_realize(), otherwise we redirect to pnv_phb4_realize(). The >>> unified device does not do anything per se other than handling control >>> to the right version. >>> >>> After this series, this same powernv8 command line that boots a powernv8 >>> machine with some phbs and root ports and with network: >>> >>> ./qemu-system-ppc64 -m 4G \ >>> -machine powernv8 -smp 2,sockets=2,cores=1,threads=1 \ >>> -accel tcg,thread=multi -bios skiboot.lid \ >>> -kernel vmlinux -initrd buildroot.rootfs.cpio -append 'console=hvc0 ro xmon=on' \ >>> -nodefaults -serial mon:stdio -nographic \ >>> -device pnv-phb,chip-id=0,index=0,id=pcie.0 \ >>> -device pnv-phb,chip-id=0,index=1,id=pcie.1 \ >>> -device pnv-phb,chip-id=1,index=2,id=pcie.2 \ >>> -device pnv-phb-root-port,id=root0,bus=pcie.2 \ >>> -device pnv-phb-root-port,id=root1,bus=pcie.1 \ >>> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \ >>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \ >>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \ >>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \ >>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \ >>> -device nec-usb-xhci,bus=bridge1,addr=0x2 >>> >>> >>> Can be used to boot powernv9 and powernv10 machines with the same attributes >>> just by changing the machine type. >>> >>> >>> Daniel Henrique Barboza (17): >>> ppc/pnv: rename PnvPHB3.ioda* to PnvPHB3.ioda2* >>> ppc/pnv: rename PnvPHB3.regs[] to PnvPHB3.regs3[] >>> ppc/pnv: rename PnvPHB3.dma_spaces to PnvPHB3.v3_dma_spaces >>> ppc/pnv: add unified pnv-phb header >>> ppc/pnv: add pnv-phb device >>> ppc/pnv: remove PnvPHB3 >>> ppc/pnv: user created pnv-phb for powernv8 >>> ppc/pnv: remove PnvPHB4 >>> ppc/pnv: user creatable pnv-phb for powernv9 >>> ppc/pnv: use PnvPHB.version >>> ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs >>> ppc/pnv: user creatable pnv-phb for powernv10 >>> ppc/pnv: add pnv_phb_get_current_machine() >>> ppc/pnv: add pnv-phb-root-port device >>> ppc/pnv: remove pnv-phb3-root-port >>> ppc/pnv: remove pnv-phb4-root-port >>> ppc/pnv: remove pecc->rp_model >>> >>> hw/pci-host/meson.build | 3 +- >>> hw/pci-host/pnv_phb.c | 258 ++++++++++++++++++++++++++++ >>> hw/pci-host/pnv_phb3.c | 256 +++++++++++----------------- >>> hw/pci-host/pnv_phb3_msi.c | 12 +- >>> hw/pci-host/pnv_phb3_pbcq.c | 8 +- >>> hw/pci-host/pnv_phb4.c | 298 ++++++++++++--------------------- >>> hw/pci-host/pnv_phb4_pec.c | 14 +- >>> hw/ppc/pnv.c | 41 ++++- >>> include/hw/pci-host/pnv_phb.h | 224 +++++++++++++++++++++++++ >>> include/hw/pci-host/pnv_phb3.h | 118 +------------ >>> include/hw/pci-host/pnv_phb4.h | 108 ++---------- >>> include/hw/ppc/pnv.h | 3 +- >>> 12 files changed, 757 insertions(+), 586 deletions(-) >>> create mode 100644 hw/pci-host/pnv_phb.c >>> create mode 100644 include/hw/pci-host/pnv_phb.h >> >> I'm completely on-board with having a proxy-like PHB device that maps to the >> correct underlying PHB version, but to me it feels like combining multiple separate >> devices into a single one is going to make things more complicated to maintain in >> the long term. > > I'm all up for a "virtual pnv-phb" device that can be used together with the > existing versioned phbs. I wasn't able to pull that off. ATB, Mark.
On 07/05/2022 21:06, Daniel Henrique Barboza wrote: > Hi, > > Since the 7.0.0 release cycle we have a desire to use the powernv > emulation with libvirt. To do that we need to enable user creatable > pnv-phb devices to allow more user customization an to avoid spamming > multiple default devices in the domain XML. In the middle of the > previous cycle we experimented with user created > pnv-phb3/pnv-phb3-root-port/pnv-phb4/pnv-phb4-root-port/pnv-phb5. The > end result, although functional, is that the user needs to deal with a > lot of versioned devices that, from the user perspective, does the same > thing. In a way we outsourced the implementation details of the PHBs > (e.g. pnv8 uses phb3, pnv9 uses phb4) to the command line. Having more > devices also puts an extra burden in the libvirt support we want to > have. > > To solve this, Cedric and Frederic gave the idea of adding a common > virtual pnv-phb device that the user can add in the command line, and > QEMU handles the details internally. Unfortunatelly this idea turned out > to be way harder than anticipated. Between creating a device that would > just forward the callbacks to the existing devices internally, creating > a PnvPHB device with the minimal attributes and making the other devices > inherit from it, and making an 'abstract' device that could be casted > for both phb3 and phb4 PHBs, all sorts of very obscure problems occured: > PHBs not being detected, interrupts not being delivered and memory > regions not being able to read/write registers. My initial impression is > that there are assumptions made both in ppc/pnv and hw/pci-host that > requires the memory region and the bus being in the same device. Even > if we somehow figure all this out, the resulting code is hacky and > annoying to maitain. > > This brings us to this series. The cleaner way I found to accomplish > what we want to do is to create real, unified pnv-phb/phb-phb-root-port > devices, and get rid of all the versioned ones. This is done by > merging all the PHB3/PHB4 attributes in unified devices. pnv_phb3* and pnv_phb4* > files end up using the same pnv-phb and phb-phb-root-port unified devices, > with the difference that pnv_phb3* only cares about version 3 attributes > and pnv_phb4* only cares about PHB4 attributes. Introducing new PHB > versions in the future will be a matter of adding any non-existent > attributes in the unified pnv-phb* devices and using them in separated > pnv_phbN* files. > > The pnv-phb implementation per se is just a router for either phb3 or > phb4 logic, done in init() and realize() time, after we check which powernv > machine we're running. If running with powernv8 we redirect control to > pnv_phb3_realize(), otherwise we redirect to pnv_phb4_realize(). The > unified device does not do anything per se other than handling control > to the right version. > > After this series, this same powernv8 command line that boots a powernv8 > machine with some phbs and root ports and with network: > > ./qemu-system-ppc64 -m 4G \ > -machine powernv8 -smp 2,sockets=2,cores=1,threads=1 \ > -accel tcg,thread=multi -bios skiboot.lid \ > -kernel vmlinux -initrd buildroot.rootfs.cpio -append 'console=hvc0 ro xmon=on' \ > -nodefaults -serial mon:stdio -nographic \ > -device pnv-phb,chip-id=0,index=0,id=pcie.0 \ > -device pnv-phb,chip-id=0,index=1,id=pcie.1 \ > -device pnv-phb,chip-id=1,index=2,id=pcie.2 \ > -device pnv-phb-root-port,id=root0,bus=pcie.2 \ > -device pnv-phb-root-port,id=root1,bus=pcie.1 \ > -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \ > -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \ > -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \ > -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \ > -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \ > -device nec-usb-xhci,bus=bridge1,addr=0x2 > > > Can be used to boot powernv9 and powernv10 machines with the same attributes > just by changing the machine type. > Hello Daniel, Thanks for trying to cleanup the phb3 and phb4 implementations! Like you I'm sure, I'm not fond of that big struct PnvPHB, which is accumulating all the states of all the PHB versions. It's likely going to grow, with most of it being unused. I was about to suggest if it was possible to at least have a union for the phb3- or phb4-specific attributes, but I'm glad to see that Mark is helping and hasn't given up trying to fix it by using a parent object. Hopefully we can make it work. Some random comments, which may or may not hold depending on how it is reworked: - IODA2 is the I/O Design Architecture used by phb3 and IODA3 is used by phb4. So while it makes sense to use the "ioda2_" prefix for phb3, it is counter-intuitive to use the "ioda_" prefix for attributes related to phb4. - the struct PnvPhb3DMASpace and struct PnvPhb4DMASpace are almost identical, save for the type of the backpointer to the PHB. It would be nice if we could merge them. Fred > Daniel Henrique Barboza (17): > ppc/pnv: rename PnvPHB3.ioda* to PnvPHB3.ioda2* > ppc/pnv: rename PnvPHB3.regs[] to PnvPHB3.regs3[] > ppc/pnv: rename PnvPHB3.dma_spaces to PnvPHB3.v3_dma_spaces > ppc/pnv: add unified pnv-phb header > ppc/pnv: add pnv-phb device > ppc/pnv: remove PnvPHB3 > ppc/pnv: user created pnv-phb for powernv8 > ppc/pnv: remove PnvPHB4 > ppc/pnv: user creatable pnv-phb for powernv9 > ppc/pnv: use PnvPHB.version > ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs > ppc/pnv: user creatable pnv-phb for powernv10 > ppc/pnv: add pnv_phb_get_current_machine() > ppc/pnv: add pnv-phb-root-port device > ppc/pnv: remove pnv-phb3-root-port > ppc/pnv: remove pnv-phb4-root-port > ppc/pnv: remove pecc->rp_model > > hw/pci-host/meson.build | 3 +- > hw/pci-host/pnv_phb.c | 258 ++++++++++++++++++++++++++++ > hw/pci-host/pnv_phb3.c | 256 +++++++++++----------------- > hw/pci-host/pnv_phb3_msi.c | 12 +- > hw/pci-host/pnv_phb3_pbcq.c | 8 +- > hw/pci-host/pnv_phb4.c | 298 ++++++++++++--------------------- > hw/pci-host/pnv_phb4_pec.c | 14 +- > hw/ppc/pnv.c | 41 ++++- > include/hw/pci-host/pnv_phb.h | 224 +++++++++++++++++++++++++ > include/hw/pci-host/pnv_phb3.h | 118 +------------ > include/hw/pci-host/pnv_phb4.h | 108 ++---------- > include/hw/ppc/pnv.h | 3 +- > 12 files changed, 757 insertions(+), 586 deletions(-) > create mode 100644 hw/pci-host/pnv_phb.c > create mode 100644 include/hw/pci-host/pnv_phb.h >
On 5/10/22 04:57, Mark Cave-Ayland wrote: > On 09/05/2022 23:30, Daniel Henrique Barboza wrote: > >> On 5/9/22 18:17, Mark Cave-Ayland wrote: >>> On 07/05/2022 20:06, Daniel Henrique Barboza wrote: >>> >>>> Hi, >>>> >>>> Since the 7.0.0 release cycle we have a desire to use the powernv >>>> emulation with libvirt. To do that we need to enable user creatable >>>> pnv-phb devices to allow more user customization an to avoid spamming >>>> multiple default devices in the domain XML. In the middle of the >>>> previous cycle we experimented with user created >>>> pnv-phb3/pnv-phb3-root-port/pnv-phb4/pnv-phb4-root-port/pnv-phb5. The >>>> end result, although functional, is that the user needs to deal with a >>>> lot of versioned devices that, from the user perspective, does the same >>>> thing. In a way we outsourced the implementation details of the PHBs >>>> (e.g. pnv8 uses phb3, pnv9 uses phb4) to the command line. Having more >>>> devices also puts an extra burden in the libvirt support we want to >>>> have. >>>> >>>> To solve this, Cedric and Frederic gave the idea of adding a common >>>> virtual pnv-phb device that the user can add in the command line, and >>>> QEMU handles the details internally. Unfortunatelly this idea turned out >>>> to be way harder than anticipated. Between creating a device that would >>>> just forward the callbacks to the existing devices internally, creating >>>> a PnvPHB device with the minimal attributes and making the other devices >>>> inherit from it, and making an 'abstract' device that could be casted >>>> for both phb3 and phb4 PHBs, >>> >>> This bit sounds all good... >>> >>>> all sorts of very obscure problems occured: >>>> PHBs not being detected, interrupts not being delivered and memory >>>> regions not being able to read/write registers. My initial impression is >>>> that there are assumptions made both in ppc/pnv and hw/pci-host that >>>> requires the memory region and the bus being in the same device. Even >>>> if we somehow figure all this out, the resulting code is hacky and >>>> annoying to maitain. >>> >>> But this seems really surprising since there are many examples of similar QOM patterns within the codebase, and in my experience they work really well. Do you have a particular example that you can share to demonstrate why the result of the QOM mapping is making things more difficult? >> >> >> It's not so much about the OOM getting in the way, but rather myself not being >> able to use QOM in a way to get what I want. There is a good probability that >> QOM is able to do it. >> >> Talking about the 2 PHBs pnv-phb3 (powernv8 only) and pnv-phb4 (powernv9 only), >> what we want is a way to let the user instantiate both using an alias, or >> an abstract object if you will, called "pnv-phb". This alias/abstraction would >> instantiate either a pnv-phb3 or a pnv-phb4 depending on the actual machine >> running. >> >> QOM has the "interface" concept that is used internally to make the device behave >> like the interface describes it, but I wasn't able to expose this kind of object >> to the user. It also has the "abstract" concept that, by the documentation itself, >> isn't supposed to be user created. Eventually I gave up this idea, accepting that >> only real devices can be exposed to the user (please correct me if I'm wrong). > > Sorry, I should have clarified this a bit more: introducing an abstract type is a separate task from adding your proxy device type, but I think will definitely help maintaining the individual versions. Certainly it will make it easier to see which fields are in use by specific versions, and it also gives you a QOM cast for the superclass of all PHB devices to help with type checking. > >> After that I tried to create a pnv-phb object that contains the common attributes >> of both pnv-phb3 and pnv-phb4. This object would be the parent of phb3 and phb4, >> and during realize time creating a pnv-phb object would create a pnv-phb3/4 and >> we go from there. This attempt went far after a few tweaks but then I started >> to hit the problems I mentioned above: some strange behaviors with interrupts, >> PHBs not appearing and so on. I got a little farther by moving all the memory >> regions from phb3/phb4 to the parent phb object and I got up to the guest login, >> but without network. Since moving the memory regions in the same object as the >> pci root bus got me farther I concluded that there might some relation/assumption >> made somewhere that I was breaking before. > > That sounds really odd. The normal reasons for this in my experience are i) forgetting to update the class/instance size in the class_init function (an --enable-sanitizers build will catch this) and ii) not propagating device reset/realize functions to a parent device leading to uninitialised state. > >> After all that I got more than halfway of what I ended up doing. I decided to >> unify phb3/phb4 into a single device, renamed their attributes that has different >> meanings between v3 and v4 code, and I ended up with this series. > > As a rough outline for a pnv-phb device, I'd aim for creating a proxy for the underlying device rather than manually invoking the QOM instance and qdev-related functions: > > > struct PnvPHB { > .... > uint32_t version; > Object *phb_dev; /* Could be PHBCommonBase if it exists */ > }; > > DECLARE_SIMPLE_OBJECT_TYPE(...) > > ... > ... > > static Property pnv_phb_properties[] = { > DEFINE_PROP_UINT32("version", PnvPHB, version, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > static void pnv_phb_realize(DeviceState *dev, Error **errp) > { > PnvPHB *pnv_phb = PNV_PHB(dev); > g_autofree char *phb_typename; > > if (!pnv_phb->version) { > error_setg("version not specified", errp); > return; > } > > switch (pnv_phb->version) { > case 3: > phb_typename = g_strdup(TYPE_PNV_PHB3); > break; > case 4: > phb_typename = g_strdup(TYPE_PNV_PHB4); > break; > default: > g_assert_unreached(); > } > > pnv_phb->phb_dev = object_new(phb_typename); > object_property_add_child(OBJECT(dev), "phb-device", pnv_phb->phb_dev); > > if (!qdev_realize(DEVICE(pnv_phb->phb_dev), errp)) { > return; > } > > /* Passthrough child device properties to the proxy device */ > qdev_alias_all_properties(dev, OBJECT(pnv_phb->phb_dev)); > } > > Finally you can set the pnv-phb version on a per-machine basis by adding the version to the machine compat_props: > > static GlobalProperty compat[] = { > { TYPE_PHB_PNV, "version", 3}, > }; That's really interesting. I'll get a spin with your suggestion to see if I can make it work. Thanks! Daniel > >>>> This brings us to this series. The cleaner way I found to accomplish >>>> what we want to do is to create real, unified pnv-phb/phb-phb-root-port >>>> devices, and get rid of all the versioned ones. This is done by >>>> merging all the PHB3/PHB4 attributes in unified devices. pnv_phb3* and pnv_phb4* >>>> files end up using the same pnv-phb and phb-phb-root-port unified devices, >>>> with the difference that pnv_phb3* only cares about version 3 attributes >>>> and pnv_phb4* only cares about PHB4 attributes. Introducing new PHB >>>> versions in the future will be a matter of adding any non-existent >>>> attributes in the unified pnv-phb* devices and using them in separated >>>> pnv_phbN* files. >>>> >>>> The pnv-phb implementation per se is just a router for either phb3 or >>>> phb4 logic, done in init() and realize() time, after we check which powernv >>>> machine we're running. If running with powernv8 we redirect control to >>>> pnv_phb3_realize(), otherwise we redirect to pnv_phb4_realize(). The >>>> unified device does not do anything per se other than handling control >>>> to the right version. >>>> >>>> After this series, this same powernv8 command line that boots a powernv8 >>>> machine with some phbs and root ports and with network: >>>> >>>> ./qemu-system-ppc64 -m 4G \ >>>> -machine powernv8 -smp 2,sockets=2,cores=1,threads=1 \ >>>> -accel tcg,thread=multi -bios skiboot.lid \ >>>> -kernel vmlinux -initrd buildroot.rootfs.cpio -append 'console=hvc0 ro xmon=on' \ >>>> -nodefaults -serial mon:stdio -nographic \ >>>> -device pnv-phb,chip-id=0,index=0,id=pcie.0 \ >>>> -device pnv-phb,chip-id=0,index=1,id=pcie.1 \ >>>> -device pnv-phb,chip-id=1,index=2,id=pcie.2 \ >>>> -device pnv-phb-root-port,id=root0,bus=pcie.2 \ >>>> -device pnv-phb-root-port,id=root1,bus=pcie.1 \ >>>> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \ >>>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \ >>>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \ >>>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \ >>>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \ >>>> -device nec-usb-xhci,bus=bridge1,addr=0x2 >>>> >>>> >>>> Can be used to boot powernv9 and powernv10 machines with the same attributes >>>> just by changing the machine type. >>>> >>>> >>>> Daniel Henrique Barboza (17): >>>> ppc/pnv: rename PnvPHB3.ioda* to PnvPHB3.ioda2* >>>> ppc/pnv: rename PnvPHB3.regs[] to PnvPHB3.regs3[] >>>> ppc/pnv: rename PnvPHB3.dma_spaces to PnvPHB3.v3_dma_spaces >>>> ppc/pnv: add unified pnv-phb header >>>> ppc/pnv: add pnv-phb device >>>> ppc/pnv: remove PnvPHB3 >>>> ppc/pnv: user created pnv-phb for powernv8 >>>> ppc/pnv: remove PnvPHB4 >>>> ppc/pnv: user creatable pnv-phb for powernv9 >>>> ppc/pnv: use PnvPHB.version >>>> ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs >>>> ppc/pnv: user creatable pnv-phb for powernv10 >>>> ppc/pnv: add pnv_phb_get_current_machine() >>>> ppc/pnv: add pnv-phb-root-port device >>>> ppc/pnv: remove pnv-phb3-root-port >>>> ppc/pnv: remove pnv-phb4-root-port >>>> ppc/pnv: remove pecc->rp_model >>>> >>>> hw/pci-host/meson.build | 3 +- >>>> hw/pci-host/pnv_phb.c | 258 ++++++++++++++++++++++++++++ >>>> hw/pci-host/pnv_phb3.c | 256 +++++++++++----------------- >>>> hw/pci-host/pnv_phb3_msi.c | 12 +- >>>> hw/pci-host/pnv_phb3_pbcq.c | 8 +- >>>> hw/pci-host/pnv_phb4.c | 298 ++++++++++++--------------------- >>>> hw/pci-host/pnv_phb4_pec.c | 14 +- >>>> hw/ppc/pnv.c | 41 ++++- >>>> include/hw/pci-host/pnv_phb.h | 224 +++++++++++++++++++++++++ >>>> include/hw/pci-host/pnv_phb3.h | 118 +------------ >>>> include/hw/pci-host/pnv_phb4.h | 108 ++---------- >>>> include/hw/ppc/pnv.h | 3 +- >>>> 12 files changed, 757 insertions(+), 586 deletions(-) >>>> create mode 100644 hw/pci-host/pnv_phb.c >>>> create mode 100644 include/hw/pci-host/pnv_phb.h >>> >>> I'm completely on-board with having a proxy-like PHB device that maps to the correct underlying PHB version, but to me it feels like combining multiple separate devices into a single one is going to make things more complicated to maintain in the long term. >> >> I'm all up for a "virtual pnv-phb" device that can be used together with the >> existing versioned phbs. I wasn't able to pull that off. > > > ATB, > > Mark.
> As a rough outline for a pnv-phb device, I'd aim for creating a proxy for the underlying device rather than manually invoking the QOM instance and qdev-related functions: > > > struct PnvPHB { > .... > uint32_t version; > Object *phb_dev; /* Could be PHBCommonBase if it exists */ > }; > > DECLARE_SIMPLE_OBJECT_TYPE(...) > > ... > ... > > static Property pnv_phb_properties[] = { > DEFINE_PROP_UINT32("version", PnvPHB, version, 0), It could be the type name directly. > DEFINE_PROP_END_OF_LIST(), > }; > > static void pnv_phb_realize(DeviceState *dev, Error **errp) > { > PnvPHB *pnv_phb = PNV_PHB(dev); > g_autofree char *phb_typename; > > if (!pnv_phb->version) { > error_setg("version not specified", errp); > return; > } > > switch (pnv_phb->version) { > case 3: > phb_typename = g_strdup(TYPE_PNV_PHB3); > break; > case 4: > phb_typename = g_strdup(TYPE_PNV_PHB4); > break; > default: > g_assert_unreached(); > } > > pnv_phb->phb_dev = object_new(phb_typename); > object_property_add_child(OBJECT(dev), "phb-device", pnv_phb->phb_dev); > > if (!qdev_realize(DEVICE(pnv_phb->phb_dev), errp)) { > return; > } > > /* Passthrough child device properties to the proxy device */ > qdev_alias_all_properties(dev, OBJECT(pnv_phb->phb_dev)); > } > > Finally you can set the pnv-phb version on a per-machine basis by adding the version to the machine compat_props: > > static GlobalProperty compat[] = { > { TYPE_PHB_PNV, "version", 3}, > }; > That looks nice. Something to try for cold plugging phbs on the machine. Thanks, C.