Re: [PATCH v2 00/18] user creatable pnv-phb3/pnv-phb4 devices

["Cédric Le Goater" <clg@kaod.org>]

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 ports
>>
>> Looking 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.