Re: [libvirt PATCH v2 1/2] qemu: remove use of (+|-)name syntax for -cpu featres
On Thu, Oct 07, 2021 at 12:36:14PM +0200, Jiri Denemark wrote:
> On Thu, Oct 07, 2021 at 11:09:19 +0100, Daniel P. Berrangé wrote:
> > On Thu, Oct 07, 2021 at 11:53:00AM +0200, Jiri Denemark wrote:
> > > 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
> > > > 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
> > > > 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é
> > > > ---
> > > > 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, , "kvm_pv_eoi",
> > > > -def->apic_eoi == VIR_TRISTATE_SWITCH_ON);
> > > > +virBufferAsprintf(, ",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().
> >
> >
Re: [libvirt PATCH v2 1/2] qemu: remove use of (+|-)name syntax for -cpu featres
On Thu, Oct 07, 2021 at 11:09:19 +0100, Daniel P. Berrangé wrote:
> On Thu, Oct 07, 2021 at 11:53:00AM +0200, Jiri Denemark wrote:
> > 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
> > > 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
> > > 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é
> > > ---
> > > 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, , "kvm_pv_eoi",
> > > -def->apic_eoi == VIR_TRISTATE_SWITCH_ON);
> > > +virBufferAsprintf(, ",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().
>
> virQEMUCapsCPUFeatureToQEMU() is a constant mapping. It makes sense
> to use it in the earlier loop, because we're getting feature names
> dynamically from the XML config.
>
> In this case though, we've just got a constant string. Seems better
> to just pass the right
Re: [libvirt PATCH v2 1/2] qemu: remove use of (+|-)name syntax for -cpu featres
On Thu, Oct 07, 2021 at 11:53:00AM +0200, Jiri Denemark wrote:
> 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
> > 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
> > 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é
> > ---
> > 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, , "kvm_pv_eoi",
> > -def->apic_eoi == VIR_TRISTATE_SWITCH_ON);
> > +virBufferAsprintf(, ",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().
virQEMUCapsCPUFeatureToQEMU() is a constant mapping. It makes sense
to use it in the earlier loop, because we're getting feature names
dynamically from the XML config.
In this case though, we've just got a constant string. Seems better
to just pass the right constant string in directly rather than send
it through this remapping.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-
Re: [libvirt PATCH v2 1/2] qemu: remove use of (+|-)name syntax for -cpu featres
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
> 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
> 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é
> ---
> 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, , "kvm_pv_eoi",
> -def->apic_eoi == VIR_TRISTATE_SWITCH_ON);
> +virBufferAsprintf(, ",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
