On 1/6/22 13:36, Daniel Henrique Barboza wrote:
On 1/6/22 05:18, Cédric Le Goater wrote:On 1/5/22 22:23, Daniel Henrique Barboza wrote:Hi, This second version was rebased with upstream and includes fixed/amended versions of relevant patches that were sent to the mailing list and aren't upstream yet. In this process 4 patches from v1 were discarded, becoming either irrelevant or squashed into others. The patches are organized as follows: - patches 1-4: enable user creatable phb3/phb4 root portsLooking closer at models and domain files in libvirt, aren't user creatable phb3/phb4 root ports enough ? Do we really need the pnv-phb3/pnv-phb4 devices to be user created also ?We need user creatable phbs for a handful of reasons. Let's suppose we go this route and all PHBs are available all the time and user just adds root-ports. Using spapr-phb as the closest example of a default PHB that's always available, we would need to express the PHBs in the XML. For a 4 socket powernv9 domain we would have 24 PHBs in the XML. No, hiding them is not an option because it would break assumptions Libvirt makes where all PCI controllers are expressed in the XML, and the controller relationship via controller indexes are awlays explicit. Having "ghost PHBs" that exists but aren't visible in the XML was something that I was having to deal with with user creatable PEC controllers and it's both complex to do and and has a good chance of getting NACKed by the community because it's too hard to use. So having these default PHBs expressed in the XML would be a must. So right off the bat, for a 1 socket powernv9 machine, we'll have 6 phbs that will need to be in the XML regardless of being used or not. And for each socket added/remove we'll have to add/remove default PHBs available in the domain XML. So if the user starts with 1 sockets, and then adds a root port, the XML can look similar to this: <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='pcie-root'/> <controller type='pci' index='2' model='pcie-root'/> <controller type='pci' index='3' model='pcie-root'/> <controller type='pci' index='4' model='pcie-root'/> <controller type='pci' index='5' model='pcie-root'/> <controller type='pci' index='6' model='pcie-root-port'/> (... user adds more controllers with index=7,8 ...) Now if the user adds another socket we have a problem. The root ports and other devices will be using controller indexes that the default PHBs would use. The user did a CPU topology change and now will have to adjust PCI topology as a result. Similar complications will happen if the user then removes a socket from the domain. Now, with user creatable PHBs, the situation above will be expressed like: <controller type='pci' index='0' model='pcie-root'/> <model name='pnv-phb4'/> <target index='4' chip-id='0'/> </controller> <controller type='pci' index='1' model='pcie-root-port'/> (... user adds more controllers with index=2,3 ...) If the user adds 4 sockets this doesn't change because Libvirt is creating a single PHB and renaming it to 'pcie.0' to be consistent. If the user decides, in a 2 socket pnv9 domain, to use the PHB that belongs to the second chip, the only change is the chip-id element: <controller type='pci' index='0' model='pcie-root'/> <model name='pnv-phb4'/> <target index='4' chip-id='1'/> </controller> <controller type='pci' index='1' model='pcie-root-port'/> (... user adds more controllers with index=2,3 ...) If the user then removes the socket the domain will error out when starting because you're creating a PHB with a wrong chip-id. A simple matter of changing the chip-id value while retaining the PCI topology as is, without needing to reassign controller indexes all over again. Another big deal is to able to rename buses. Libvirt uses 'pcie.N' with these controllers, we're using 'pnv-phb3/4-root-bus'. Without user creatable PHBs we would be signing a contract with Libvirt that we will never rename these buses in QEMU side again, once Libvirt starts support it, because now Libvirt is counting on this info to correctly assign the root ports to the specific PHBs. Being able to rename the buses is also crucial for the PCI topology consistency I commented above. There's also a good argument about long term extensibility. We're doing a lot of work in the QEMU side but we'll be able to later on the road, for instance, support multiple root-ports in the same PHB, or even devices other than root-ports in PHBs, device hotplug and so on with minimal - and most important, backward friendly - Libvirt changes. Having default PHBs appearing all the time will force us to make several assumptions that we wouldn't be able to break later on, and probably will break older domains that were created before the changes. I am probably forgetting more problems that this would cause in Libvirt. But at last, but definitely not the least for the implementation of the Libvirt side, we (in this case, I) would need to make lots and lots of code to support default PHBs that can be added/removed via SMP changes, while trying to keep existing PCI topologies minimally consistent, with lots of documentation explaining why are we adding/removing default PHBs due to SMP changes, and in the end the user experience would still be awkward. Enabling user creatable pnv-phb3/phb4 is the way for Libvirt support, and I daresay it also makes for an improved QEMU experience as well. If I want a -nodefaults machine with 4 sockets but only 2 PHBs I can do that after this series. Decopling SMP from PCI topology has advantages outside of Libvirt support.
Thanks for this detailed explanation. I am convinced !
- patches 5-10: enable user creatable pnv-phb3 devices
These are fine. No need to resend.
- patches 11-18: enable user creatable pnv-phb4 devices
patches 11-17 need some care. 18 is fine.
Here are some examples of what we're able to do with this series: * powernv8 machine with -nodefaults,2 pnv-phb3s with 'pcie.N' name, one of them with a root port and a netcard: $ qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg -smp 2,cores=2,threads=1 \ -bios skiboot.lid -kernel vmlinux -initrd buildroot.rootfs.cpio \ -append 'console=hvc0 ro xmon=on' \ -nodefaults \ -serial mon:stdio -nographic \ -device pnv-phb3,chip-id=0,index=0,id=pcie.0 \ -device pnv-phb3,chip-id=0,index=2,id=pcie.2 \ -device pnv-phb3-root-port,bus=pcie.2,id=pcie.5 \ -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \ -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=pcie.5,addr=0x0 * powernv9 machine with -nodefaults, 3 of the available 12 pnv-phb4 devices created, 2 root ports, one of the port with a pcie-pci-bridge and devices connected in the bridge: $ qemu-system-ppc64 -m 4G -machine powernv9 \ -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-phb4,chip-id=0,index=0,id=pcie.0 \ -device pnv-phb4,chip-id=0,index=4,id=pcie.1 \ -device pnv-phb4,chip-id=1,index=3,id=pcie.2 \ -device pnv-phb4-root-port,id=root0,bus=pcie.2 \ -device pnv-phb4-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 * powernv8/9 with default settings can be used as usual. The work done in this series didn't change the name of the buses created by the default root ports (named pcie.0...N): $ qemu-system-ppc64 -m 4G \ -machine powernv9 -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' \ -serial mon:stdio -nographic \ -device pcie-pci-bridge,id=bridge1,bus=pcie.0,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
Could we capture some of these command lines in the documentation ? with some of the details above ? Thanks, C.
