Re: [PATCH v3 4/5] qemu: add support for Direct Mode for Hyper-V Synthetic timers

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Fri, Aug 09, 2019 at 12:13:45PM +0200, Vitaly Kuznetsov wrote:
> Ján Tomko <jtomko@xxxxxxxxxx> writes:
>
> > One more thing, qemuxml2argvtest is using DO_TEST:
> >     DO_TEST("hyperv", NONE);
> >     DO_TEST("hyperv-off", NONE);
> >     DO_TEST("hyperv-panic", NONE);
> > which manually enumarate the used QEMU capabilities (although there are
> > none here)
> >
> > DO_TEST_CAPS_LATEST is the way to test against QEMU capabilites
> > gathered from a real binary - if you use that macro for testing,
> > it will have the QEMU_CAPS_CANONICAL_CPU_FEATURES capability and all the
> > included features will use '-' instead of '_' as the separator.
>
> Forgive me my ignorance (and long reply times), is the idea aimed at
> removing "hvPrefix" machinery from qemuBuildCpuCommandLine() completely
> - probably in favor of hardcorded '-' delimiters?

For new features that will not be used with QEMU <4.1, which does
not have QEMU_CAPS_CANONICAL_CPU_FEATURES we should hardcode the version
with the '-' delimiter.

> I tried switching to DO_TEST_CAPS_LATEST in qemuxml2argvtest and after
> renaming
> tests/qemuxml2argvdata/hyperv{-off,-panic}.args to
> tests/qemuxml2argvdata/hyperv{,-off,-panic}.x86_64-latest.args
> to make the test run, I'm getting the following:
>
> In '/home/vitty/workspace/Upstream/libvirt/tests/qemuxml2argvdata/hyperv.x86_64-latest.args':
> Offset 292
> Expect [QEMUGuest1 -S -machine pc,accel=tcg,usb=off,dump-guest-core=off -cpu 'qemu32,hv-relaxed,hv-vapic,hv-spinlocks=0x2fff,hv-vpindex,hv-runtime,hv-synic,hv-stimer,hv-reset,hv-vendor-id=KVM Hv,hv-frequencies,hv-reenlightenment,hv-tlbflush,hv-ipi,hv-evmcs' -m 214 -realtime mlock=off -smp 6,sockets=6,cores=1,threads=1 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 -display none -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown -usb]
> Actual [guest=QEMUGuest1,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes -machine pc,accel=tcg,usb=off,dump-guest-core=off -cpu 'qemu32,hv-relaxed,hv-vapic,hv-spinlocks=0x2fff,hv-vpindex,hv-runtime,hv-synic,hv-stimer,hv-reset,hv-vendor-id=KVM Hv,hv-frequencies,hv-reenlightenment,hv-tlbflush,hv-ipi,hv-evmcs' -m 214 -overcommit mem-lock=off -smp 6,sockets=6,cores=1,threads=1 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 -display none -no-user-config -nodefaults -chardev socket,id=charmonitor,fd=1729,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown -boot strict=on -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg timestamp=on]

From a glance all these changes look OK and are a result of using newer
syntax for the same options.

To easily regenerate the output, set the env variable VIR_TEST_REGENERATE_OUTPUT=1 before running
the test.

> I can, probably, add all the missing stuff but I'm not sure it's the
> right way to go in the first place :-)
>
> What if we just add QEMU_CAPS_CANONICAL_CPU_FEATURES to DO_TEST:

We try to steer away from enumerating capabilites in the tests.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 71a36ff63a..f88d45f4e6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7152,11 +7152,6 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
>     }
>
>     if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) {
> -        const char *hvPrefix = "hv-";
> -
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CANONICAL_CPU_FEATURES))
> -            hvPrefix = "hv_";
> -

IIUC we still need this code to deal with older QEMU. It's just new
QEMUs where we can use the unified '-' delimiter.

>         for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) {
>             switch ((virDomainHyperv) i) {
>             case VIR_DOMAIN_HYPERV_RELAXED:
> @@ -7172,8 +7167,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
>             case VIR_DOMAIN_HYPERV_IPI:
>             case VIR_DOMAIN_HYPERV_EVMCS:
>                 if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON)
> -                    virBufferAsprintf(&buf, ",%s%s",
> -                                      hvPrefix,
> +                    virBufferAsprintf(&buf, ",hv-%s",
>                                       virDomainHypervTypeToString(i));
>                 break;
>
> diff --git a/tests/qemuxml2argvdata/hyperv.args b/tests/qemuxml2argvdata/hyperv.args
> index 086adaa349..d09d8acdff 100644
> --- a/tests/qemuxml2argvdata/hyperv.args
> +++ b/tests/qemuxml2argvdata/hyperv.args
> @@ -11,9 +11,9 @@ QEMU_AUDIO_DRV=none \
> -name QEMUGuest1 \
> -S \
> -machine pc,accel=tcg,usb=off,dump-guest-core=off \
> --cpu 'qemu32,hv_relaxed,hv_vapic,hv-spinlocks=0x2fff,hv_vpindex,hv_runtime,\
> -hv_synic,hv_stimer,hv_reset,hv-vendor-id=KVM Hv,hv_frequencies,\
> -hv_reenlightenment,hv_tlbflush,hv_ipi,hv_evmcs' \
> +-cpu 'qemu32,hv-relaxed,hv-vapic,hv-spinlocks=0x2fff,hv-vpindex,hv-runtime,\
> +hv-synic,hv-stimer,hv-reset,hv-vendor-id=KVM Hv,hv-frequencies,\
> +hv-reenlightenment,hv-tlbflush,hv-ipi,hv-evmcs' \
> -m 214 \
> -realtime mlock=off \
> -smp 6,sockets=6,cores=1,threads=1 \
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index c166fd18d6..d13c7c8b56 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -911,9 +911,9 @@ mymain(void)
>     DO_TEST_CAPS_VER("kvmclock+eoi-disabled", "4.0.0");
>     DO_TEST_CAPS_LATEST("kvmclock+eoi-disabled");
>
> -    DO_TEST("hyperv", NONE);
> -    DO_TEST("hyperv-off", NONE);
> -    DO_TEST("hyperv-panic", NONE);

This would remove the coverage of the syntax for old QEMUs.
Either leave this as-is and add the new feature in a separate test,
or you can branch the test based on QEMU versions like we do for other
features:

   DO_TEST_CAPS_VER("pv-spinlock-disabled", "2.7.0");
   DO_TEST_CAPS_VER("pv-spinlock-disabled", "4.0.0");
   DO_TEST_CAPS_LATEST("pv-spinlock-disabled");

(After 2.7.0 we stopped using +|-feature in favor of feature=on|off,
and 4.0.0 is the last one where we use underscores)

Jano

> +    DO_TEST("hyperv", QEMU_CAPS_CANONICAL_CPU_FEATURES);
> +    DO_TEST("hyperv-off", QEMU_CAPS_CANONICAL_CPU_FEATURES);
> +    DO_TEST("hyperv-panic", QEMU_CAPS_CANONICAL_CPU_FEATURES);
>
>     DO_TEST("kvm-features", NONE);
>     DO_TEST("kvm-features-off", NONE);
>
> --
> Vitaly
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list