[PATCH v2 09/29] qemu: introduce QEMU_CAPS_DEVICE_PNV_PHB3

[Daniel Henrique Barboza]

On 2/21/22 10:07, Ján Tomko wrote:
> On a Tuesday in 2022, Daniel Henrique Barboza wrote:
>> QEMU_CAPS_DEVICE_PNV_PHB3 indicates binary support for the pnv-phb3
>> device, the pcie-root controller for PowerNV8 domains, and also
>> the pnv-phb3-root-port device, its pcie-root-port device.
>>
>> This capability is present in QEMU since 5.0.0 but these devices are
>> user-creatable only after QEMU 6.2.0. This means that probing it as
>> default will be misleading for users. Instead, let's use
>> virQEMUCapsInitQMPVersionCaps() to check for the adequate QEMU version
>> and arch and set it manually.
>>
>> Suggested-by: Peter Krempa <pkrempa at redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>> src/qemu/qemu_capabilities.c                  | 19 +++++++++++++++++++
>> src/qemu/qemu_capabilities.h                  |  1 +
>> .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml |  1 +
>> 3 files changed, 21 insertions(+)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index bb90715569..d60240912c 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -662,6 +662,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>>               /* 420 */
>>               "device.json+hotplug", /* QEMU_CAPS_DEVICE_JSON */
>>               "hvf", /* QEMU_CAPS_HVF */
>> +              "pnv-phb3", /* QEMU_CAPS_DEVICE_PNV_PHB3 */
>>     );
>>
>>
>> @@ -1397,6 +1398,17 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
>>     { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
>>     { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
>>     { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
>> +    /*
>> +     * We don't probe the following PowerNV devices:
>> +     *
> 
> If we don't probe them, then this comment does not belong here. Rather
> it should be documented in the commit message and the place that sets it
> based on the version.

Ok.

> 
>> +     * { "pnv-phb3", QEMU_CAPS_DEVICE_PNV_PHB3 },
>> +     * { "pnv-phb3-root-port", QEMU_CAPS_DEVICE_PNV_PHB3 },
>> +     *
>> +     * Because they are present in QEMU binaries since QEMU 5.0.0
>> +     * but became user creatable only in the QEMU 7.0.0 development
>> +     * cycle. Their respective capabilities are being set in
>> +     * virQEMUCapsInitQMPVersionCaps().
> 
> Alternatively, you can probe the device here (just "pnv-phb3", no need
> to look at both) and clear the capability if QEMU is too old to let the
> user create it. That way it will be easier to delete years from now when
> we raise minimum QEMU version.

Got it.

> 
>> +     */
>> };
>>
>>
>> @@ -5231,6 +5243,13 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCaps *qemuCaps)
>>      */
>>     if (qemuCaps->version < 5002000)
>>         virQEMUCapsSet(qemuCaps, QEMU_CAPS_ENABLE_FIPS);
>> +
>> +    /* PowerNV pnv-phb devices weren't user creatable up to
>> +     * QEMU 6.2.0. The version value set here was taken from a
>> +     * binary post 6.2.0 release that has user creatable pnv-phb
>> +     * support. */
>> +    if (qemuCaps->version >= 6002050 && ARCH_IS_PPC64(qemuCaps->arch))
> 
> I'd rather not use the git version in here.
> 
> If nothing better, at least rewrite this to: > 6002000

I'll change to 6002000 and, together with the change you proposed above, we will
assume that the caps exists and remove them if we're running with QEMU 6.2.0 or
older.

This support went live right after 6.2.0 released, so even if in theory there is some
QEMU builds where this assumption would fail, it is highly unlikely for one to grab a
post 6.2.0 QEMU commit that happens to not have it today.

> 
> Even better if we waited for 7.0.0 or QEMU had some property we could

What if I bump the minimal version to 7.0.0 after QEMU 7.0.0 is released?

This would prevent these series to be postponed for another 2 months, we would cover the
small version gap that I mentioned above and I can proceed with adding more devices
on top of it (e.g. the BMC devices).

> look for (looking at QEMU history there were some properties removed
> for phb4 as a part of making them user creatable but I don't see
> anything for phb3).

There's a bit of QEMU lore here but long story short: both powernv8 and powernv9 machines
weren't libvirt material (the PHBs weren't user creatable, so -nodefaults would create
a domain without any PHBs).

Upstream QEMU made post 6.2.0 changes to make it happen. The changes you are referring
to were part of this process. The powernv9 machine was a little bloated and we took this
opportunity to clean up the model.


Thanks,


Daniel


> 
>> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_PNV_PHB3);
>> }
>>
>>
> 
> With the comment removed and/or the code probing first then clearing
> based on version number:
> 
> Reviewed-by: Ján Tomko <jtomko at redhat.com>
> 
> Jano