[libvirt PATCH v2 1/2] qemu: remove use of (+|-)name syntax for -cpu featres

[Jiri Denemark]

On Thu, Oct 07, 2021 at 10:05:12 +0100, Daniel P. Berrangé wrote:
> The -cpu arg gained support for feature=on|off syntax for the x86
> emulator in 2.4.0
> 
>   commit 38e5c119c2925812bd441450ab9e5e00fc79e662
>   Author: Eduardo Habkost <ehabkost at redhat.com>
>   Date:   Mon Mar 23 17:29:32 2015 -0300
> 
>     target-i386: Register QOM properties for feature flags
> 
> Most other targets gained this syntax even earlier in 1.4.1
> 
>   commit 1590bbcb02921dfe8e3cf66e3a3aafd31193babf
>   Author: Andreas Färber <afaerber at suse.de>
>   Date:   Mon Mar 3 23:33:51 2014 +0100
> 
>     cpu: Implement CPUClass::parse_features() for the rest of CPUs
> 
>     CPUs who do not provide their own implementation of feature parsing
>     will treat each option as a QOM property and set it to the supplied
>     value.
> 
> There appears no reason to keep supporting "+|-feature" syntax,
> given the current minimum QEMU version.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/qemu/qemu_command.c                       | 34 ++++++-------------
>  tests/qemuxml2argvdata/cpu-Haswell2.args      |  2 +-
>  tests/qemuxml2argvdata/cpu-Haswell3.args      |  2 +-
>  .../qemuxml2argvdata/cpu-cache-disable3.args  |  2 +-
>  .../cpu-check-default-partial.args            |  2 +-
>  tests/qemuxml2argvdata/cpu-eoi-disabled.args  |  2 +-
>  tests/qemuxml2argvdata/cpu-eoi-enabled.args   |  2 +-
>  tests/qemuxml2argvdata/cpu-exact1.args        |  2 +-
>  .../cpu-exact2-nofallback.args                |  2 +-
>  tests/qemuxml2argvdata/cpu-exact2.args        |  2 +-
>  tests/qemuxml2argvdata/cpu-fallback.args      |  2 +-
>  tests/qemuxml2argvdata/cpu-host-kvmclock.args |  2 +-
>  .../qemuxml2argvdata/cpu-host-model-cmt.args  |  2 +-
>  .../cpu-host-model-fallback.args              |  2 +-
>  .../cpu-host-model-vendor.args                |  2 +-
>  tests/qemuxml2argvdata/cpu-host-model.args    |  2 +-
>  .../cpu-host-passthrough-features.args        |  2 +-
>  tests/qemuxml2argvdata/cpu-kvmclock.args      |  2 +-
>  tests/qemuxml2argvdata/cpu-minimum1.args      |  2 +-
>  tests/qemuxml2argvdata/cpu-minimum2.args      |  2 +-
>  tests/qemuxml2argvdata/cpu-strict1.args       |  2 +-
>  .../cpu-translation.x86_64-latest.args        |  2 +-
>  tests/qemuxml2argvdata/cpu-tsc-frequency.args |  2 +-
>  .../eoi-disabled.x86_64-latest.args           |  2 +-
>  .../eoi-enabled.x86_64-latest.args            |  2 +-
>  .../graphics-spice-timeout.args               |  2 +-
>  .../kvmclock+eoi-disabled.x86_64-latest.args  |  2 +-
>  tests/qemuxml2argvdata/kvmclock.args          |  2 +-
>  .../pci-bridge-many-disks.args                |  2 +-
>  .../pv-spinlock-disabled.x86_64-latest.args   |  2 +-
>  .../pv-spinlock-enabled.x86_64-latest.args    |  2 +-
>  31 files changed, 41 insertions(+), 53 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eaa1e0deb9..0f1cdd9372 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6242,21 +6242,6 @@ qemuBuildGlobalControllerCommandLine(virCommand *cmd,
>  }
>  
>  
> -static void
> -qemuBuildCpuFeature(virQEMUCaps *qemuCaps,
> -                    virBuffer *buf,
> -                    const char *name,
> -                    bool state)
> -{
> -    name = virQEMUCapsCPUFeatureToQEMU(qemuCaps, name);
> -
> -    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
> -        virBufferAsprintf(buf, ",%s=%s", name, state ? "on" : "off");
> -    else
> -        virBufferAsprintf(buf, ",%c%s", state ? '+' : '-', name);
> -}

I guess it would have been easier and perhaps clearer to just remove the
else branch and the test itself, because...

> @@ -6433,13 +6420,14 @@ qemuBuildCpuCommandLine(virCommand *cmd,
>      }
>  
>      if (def->apic_eoi) {
> -        qemuBuildCpuFeature(qemuCaps, &buf, "kvm_pv_eoi",
> -                            def->apic_eoi == VIR_TRISTATE_SWITCH_ON);
> +        virBufferAsprintf(&buf, ",kvm_pv_eoi=%s", def->apic_eoi ==
> +                          VIR_TRISTATE_SWITCH_ON ? "on" : "off");

This is affected by the same issue spotted by Peter in v1. In other
words, when replacing qemuBuildCpuFeature you need to make sure the
feature name goes through virQEMUCapsCPUFeatureToQEMU().

...
> diff --git a/tests/qemuxml2argvdata/pv-spinlock-disabled.x86_64-latest.args b/tests/qemuxml2argvdata/pv-spinlock-disabled.x86_64-latest.args
> index a7a107f4b8..2188ff477d 100644
> --- a/tests/qemuxml2argvdata/pv-spinlock-disabled.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/pv-spinlock-disabled.x86_64-latest.args
> @@ -11,7 +11,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
>  -S \
>  -object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \
>  -machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
> --cpu qemu64,kvm-pv-unhalt=off \
> +-cpu qemu64,kvm_pv_unhalt=off \
>  -m 214 \
>  -object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
>  -overcommit mem-lock=off \
> diff --git a/tests/qemuxml2argvdata/pv-spinlock-enabled.x86_64-latest.args b/tests/qemuxml2argvdata/pv-spinlock-enabled.x86_64-latest.args
> index b217034eb0..a4f1eaedbc 100644
> --- a/tests/qemuxml2argvdata/pv-spinlock-enabled.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/pv-spinlock-enabled.x86_64-latest.args
> @@ -11,7 +11,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
>  -S \
>  -object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \
>  -machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
> --cpu qemu64,kvm-pv-unhalt=on \
> +-cpu qemu64,kvm_pv_unhalt=on \
>  -m 214 \
>  -object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
>  -overcommit mem-lock=off \

Jirka