On 3/24/22 10:48, Paolo Bonzini wrote:
> QEMU 7.0.0 adds a new property tsc-clear-on-reset to x86 CPU, corresponding
> to Libvirt's <tsc on_reboot="clear"/> element. Plumb it in the validation,
> command line handling and tests.
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 4 +++
> src/qemu/qemu_validate.c | 36 +++++++++++++------
> .../caps_7.0.0.x86_64.replies | 4 +++
> .../caps_7.0.0.x86_64.xml | 1 +
> ...uency.args => cpu-tsc-clear-on-reset.args} | 2 +-
> ... cpu-tsc-clear-on-reset.x86_64-7.0.0.args} | 6 ++--
> ...equency.xml => cpu-tsc-clear-on-reset.xml} | 2 +-
> tests/qemuxml2argvtest.c | 2 ++
> 10 files changed, 44 insertions(+), 16 deletions(-)
> copy tests/qemuxml2argvdata/{cpu-tsc-frequency.args => cpu-tsc-clear-on-reset.args} (97%)
> copy tests/qemuxml2argvdata/{virtio-rng-builtin.x86_64-latest.args => cpu-tsc-clear-on-reset.x86_64-7.0.0.args} (77%)
> copy tests/qemuxml2argvdata/{cpu-tsc-frequency.xml => cpu-tsc-clear-on-reset.xml} (95%)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 32980e7330..d22bbee70e 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -668,6 +668,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>
> /* 425 */
> "blockdev.nbd.tls-hostname", /* QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME */
> + "x86-cpu.tsc-clear-on-reset", /* QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET */
> );
>
>
> @@ -1775,6 +1776,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendMemfd[]
> static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMaxCPU[] = {
> { "unavailable-features", QEMU_CAPS_CPU_UNAVAILABLE_FEATURES },
> { "kvm-no-adjvtime", QEMU_CAPS_CPU_KVM_NO_ADJVTIME },
> + { "tsc-clear-on-reset", QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET },
> { "migratable", QEMU_CAPS_CPU_MIGRATABLE },
> };
>
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 0a215a11d5..7aee73a725 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -643,6 +643,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
>
> /* 425 */
> QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME, /* tls hostname can be overriden for NBD clients */
> + QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET, /* x86-cpu.tsc-clear-on-reset */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c836799888..8ecede0ade 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6613,6 +6613,10 @@ qemuBuildCpuCommandLine(virCommand *cmd,
> case VIR_DOMAIN_TIMER_NAME_TSC:
> if (timer->frequency > 0)
> virBufferAsprintf(&buf, ",tsc-frequency=%llu", timer->frequency);
> + if (timer->reboot == VIR_DOMAIN_TIMER_REBOOT_MODE_CLEAR)
> + virBufferAddLit(&buf, ",tsc-clear-on-reset=on");
> + else if (timer->reboot == VIR_DOMAIN_TIMER_REBOOT_MODE_KEEP)
> + virBufferAddLit(&buf, ",tsc-clear-on-reset=off");
Here, I'd prefer switch(), esp. after the reboot member is declared as
corresponding enum instead of int so that compiler warns us about this
place when a new enum value is added.
> break;
> case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
> switch (timer->tickpolicy) {
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index f27e480696..c4ab2976dc 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -396,10 +396,11 @@ static int
> qemuValidateDomainDefClockTimers(const virDomainDef *def,
> virQEMUCaps *qemuCaps)
> {
> + virDomainTimerDef *timer;
> size_t i;
>
> for (i = 0; i < def->clock.ntimers; i++) {
> - virDomainTimerDef *timer = def->clock.timers[i];
> + timer = def->clock.timers[i];
>
> switch ((virDomainTimerNameType)timer->name) {
> case VIR_DOMAIN_TIMER_NAME_PLATFORM:
> @@ -409,20 +410,25 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def,
> return -1;
>
> case VIR_DOMAIN_TIMER_NAME_TSC:
> - case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
> - case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
> - if (!ARCH_IS_X86(def->os.arch) && timer->present == VIR_TRISTATE_BOOL_YES) {
> + if (!ARCH_IS_X86(def->os.arch) && timer->present == VIR_TRISTATE_BOOL_YES)
> + goto no_support;
> +
> + if (timer->reboot != VIR_DOMAIN_TIMER_REBOOT_MODE_DEFAULT &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Configuring the '%s' timer is not supported "
> - "for virtType=%s arch=%s machine=%s guests"),
> - virDomainTimerNameTypeToString(timer->name),
> - virDomainVirtTypeToString(def->virtType),
> - virArchToString(def->os.arch),
> - def->os.machine);
> + _("Configuring the '%s' timer to reset on reboot is not supported "
> + "with this QEMU binary"),
Error messages are exempt from the 80 chars rule, so that they can be
easily 'git grep'-ped.
> + virDomainTimerNameTypeToString(timer->name));
> return -1;
> }
> break;
>
> + case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
> + case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
> + if (!ARCH_IS_X86(def->os.arch) && timer->present == VIR_TRISTATE_BOOL_YES)
> + goto no_support;
> + break;
> +
> case VIR_DOMAIN_TIMER_NAME_LAST:
> break;
>
> @@ -540,6 +546,16 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def,
> }
>
> return 0;
> +
> + no_support:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Configuring the '%s' timer is not supported "
> + "for virtType=%s arch=%s machine=%s guests"),
> + virDomainTimerNameTypeToString(timer->name),
> + virDomainVirtTypeToString(def->virtType),
> + virArchToString(def->os.arch),
> + def->os.machine);
> + return -1;
> }
Michal
