From: Fabiano Rosas <farosas@suse.de>
To: Peter Maydell <peter.maydell@linaro.org>, qemu-devel@nongnu.org
Cc: Paolo Bonzini <pbonzini@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
Laurent Vivier <lvivier@redhat.com>
Subject: Re: [PATCH 1/2] tests/qtest/test-x86-cpuid-compat: Switch test_cpuid_prop to data-driven
Date: Tue, 10 Mar 2026 17:18:23 -0300 [thread overview]
Message-ID: <87tsunzbe8.fsf@suse.de> (raw)
In-Reply-To: <20260308185421.972734-2-peter.maydell@linaro.org>
Peter Maydell <peter.maydell@linaro.org> writes:
> In test-x86-cpuid-compat, we allocate memory in add_cpuid_test()
> which is then freed in the test function test_cpuid_prop(). This
> means that this memory is leaked if the test is never run, which
> happens in several cases:
> * user asks to list tests with -l option
> * user asks to run only some tests with -p or similar option
> * we are running a "subprocess" test and glib re-invokes the
> test binary telling it to run a specific single test
>
> As noted in the commit message of commit 93ed7d330321dc, we cannot
> deal with this by using g_test_add_data_func_full() and passing a
> free-function, because glib doesn't call that if the test is not
> executed.
>
> Instead, move all the data allocations into test_cpuid_prop(),
> and pass it a pointer to a struct with the necessary parameters
> which is a compile-time constant.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> tests/qtest/test-x86-cpuid-compat.c | 296 ++++++++++++++++------------
> 1 file changed, 173 insertions(+), 123 deletions(-)
>
> diff --git a/tests/qtest/test-x86-cpuid-compat.c b/tests/qtest/test-x86-cpuid-compat.c
> index be67631a85..fc90a46d37 100644
> --- a/tests/qtest/test-x86-cpuid-compat.c
> +++ b/tests/qtest/test-x86-cpuid-compat.c
> @@ -44,19 +44,43 @@ static bool qom_get_bool(const char *path, const char *prop)
> }
>
> typedef struct CpuidTestArgs {
> - const char *cmdline;
> + /* test name */
> + const char *name;
> + /* CPU type */
> + const char *cpu;
> + /* CPU features (may be NULL) */
> + const char *cpufeat;
> + /* machine type (may be NULL to use default machine) */
> + const char *machine;
> + /* CPU property to read */
> const char *property;
> + /* expected value of the property */
> int64_t expected_value;
> } CpuidTestArgs;
>
> static void test_cpuid_prop(const void *data)
> {
> const CpuidTestArgs *args = data;
> + char *cmdline;
> + char *save;
> char *path;
> QNum *value;
> int64_t val;
>
> - qtest_start(args->cmdline);
> + cmdline = g_strdup_printf("-cpu %s", args->cpu);
> +
> + if (args->cpufeat) {
> + save = cmdline;
> + cmdline = g_strdup_printf("%s,%s", cmdline, args->cpufeat);
> + g_free(save);
> + }
> + if (args->machine) {
> + save = cmdline;
> + cmdline = g_strdup_printf("-machine %s %s", args->machine, cmdline);
> + g_free(save);
> + }
> +
> + qtest_start(cmdline);
> path = get_cpu0_qom_path();
> value = qobject_to(QNum, qom_get(path, args->property));
> g_assert(qnum_get_try_int(value, &val));
> @@ -65,40 +89,9 @@ static void test_cpuid_prop(const void *data)
>
> qobject_unref(value);
> g_free(path);
> - g_free((void *)args->cmdline);
> - g_free((void *)data);
> + g_free(cmdline);
> }
>
> -static void add_cpuid_test(const char *name, const char *cpu,
> - const char *cpufeat, const char *machine,
> - const char *property, int64_t expected_value)
> -{
> - CpuidTestArgs *args = g_new0(CpuidTestArgs, 1);
> - char *cmdline;
> - char *save;
> -
> - if (!qtest_has_cpu_model(cpu)) {
> - return;
> - }
> - cmdline = g_strdup_printf("-cpu %s", cpu);
> -
> - if (cpufeat) {
> - save = cmdline;
> - cmdline = g_strdup_printf("%s,%s", cmdline, cpufeat);
> - g_free(save);
> - }
> - if (machine) {
> - save = cmdline;
> - cmdline = g_strdup_printf("-machine %s %s", machine, cmdline);
> - g_free(save);
> - }
> - args->cmdline = cmdline;
> - args->property = property;
> - args->expected_value = expected_value;
> - qtest_add_data_func(name, args, test_cpuid_prop);
> -}
> -
> -
> /* Parameters to a add_feature_test() test case */
> typedef struct FeatureTestArgs {
> /* cmdline to start QEMU */
> @@ -249,6 +242,145 @@ static void test_plus_minus(void)
> g_test_trap_assert_stdout("");
> }
>
> +static const CpuidTestArgs cpuid_tests[] = {
> + /* Original level values for CPU models: */
> + {
> + "x86/cpuid/phenom/level",
> + "phenom", NULL, NULL, "level", 5,
> + },
> + {
> + "x86/cpuid/Conroe/level",
> + "Conroe", NULL, NULL, "level", 10,
> + },
> + {
> + "x86/cpuid/SandyBridge/level",
> + "SandyBridge", NULL, NULL, "level", 0xd,
> + },
> + {
> + "x86/cpuid/486/xlevel",
> + "486", NULL, NULL, "xlevel", 0,
> + },
> + {
> + "x86/cpuid/core2duo/xlevel",
> + "core2duo", NULL, NULL, "xlevel", 0x80000008,
> + },
> + {
> + "x86/cpuid/phenom/xlevel",
> + "phenom", NULL, NULL, "xlevel", 0x8000001A,
> + },
> + {
> + "x86/cpuid/athlon/xlevel",
> + "athlon", NULL, NULL, "xlevel", 0x80000008,
> + },
> + /* If level is not large enough, it should increase automatically: */
> + /* CPUID[6].EAX: */
> + {
> + "x86/cpuid/auto-level/486/arat",
> + "486", "arat=on", NULL, "level", 6,
> + },
> + /* CPUID[EAX=7,ECX=0].EBX: */
> + {
> + "x86/cpuid/auto-level/phenom/fsgsbase",
> + "phenom", "fsgsbase=on", NULL, "level", 7,
> + },
> + /* CPUID[EAX=7,ECX=0].ECX: */
> + {
> + "x86/cpuid/auto-level/phenom/avx512vbmi",
> + "phenom", "avx512vbmi=on", NULL, "level", 7,
> + },
> + /* CPUID[EAX=0xd,ECX=1].EAX: */
> + {
> + "x86/cpuid/auto-level/phenom/xsaveopt",
> + "phenom", "xsaveopt=on", NULL, "level", 0xd,
> + },
> + /* CPUID[8000_0001].EDX: */
> + {
> + "x86/cpuid/auto-xlevel/486/3dnow",
> + "486", "3dnow=on", NULL, "xlevel", 0x80000001,
> + },
> + /* CPUID[8000_0001].ECX: */
> + {
> + "x86/cpuid/auto-xlevel/486/sse4a",
> + "486", "sse4a=on", NULL, "xlevel", 0x80000001,
> + },
> + /* CPUID[8000_0007].EDX: */
> + {
> + "x86/cpuid/auto-xlevel/486/invtsc",
> + "486", "invtsc=on", NULL, "xlevel", 0x80000007,
> + },
> + /* CPUID[8000_000A].EDX: */
> + {
> + "x86/cpuid/auto-xlevel/486/npt",
> + "486", "svm=on,npt=on", NULL, "xlevel", 0x8000000A,
> + },
> + /* CPUID[C000_0001].EDX: */
> + {
> + "x86/cpuid/auto-xlevel2/phenom/xstore",
> + "phenom", "xstore=on", NULL, "xlevel2", 0xC0000001,
> + },
> + /* SVM needs CPUID[0x8000000A] */
> + {
> + "x86/cpuid/auto-xlevel/athlon/svm",
> + "athlon", "svm=on", NULL, "xlevel", 0x8000000A,
> + },
> + /* If level is already large enough, it shouldn't change: */
> + {
> + "x86/cpuid/auto-level/SandyBridge/multiple",
> + "SandyBridge", "arat=on,fsgsbase=on,avx512vbmi=on",
> + NULL, "level", 0xd,
> + },
> + /* If level is explicitly set, it shouldn't change: */
> + {
> + "x86/cpuid/auto-level/486/fixed/0xF",
> + "486",
> + "level=0xF,arat=on,fsgsbase=on,avx512vbmi=on,xsaveopt=on",
> + NULL, "level", 0xF,
> + },
> + {
> + "x86/cpuid/auto-level/486/fixed/2",
> + "486",
> + "level=2,arat=on,fsgsbase=on,avx512vbmi=on,xsaveopt=on",
> + NULL, "level", 2,
> + },
> + {
> + "x86/cpuid/auto-level/486/fixed/0",
> + "486",
> + "level=0,arat=on,fsgsbase=on,avx512vbmi=on,xsaveopt=on",
> + NULL, "level", 0,
> + },
> + /* if xlevel is already large enough, it shouldn't change: */
> + {
> + "x86/cpuid/auto-xlevel/phenom/3dnow",
> + "phenom", "3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> + NULL, "xlevel", 0x8000001A,
> + },
> + /* If xlevel is explicitly set, it shouldn't change: */
> + {
> + "x86/cpuid/auto-xlevel/486/fixed/80000002",
> + "486",
> + "xlevel=0x80000002,3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> + NULL, "xlevel", 0x80000002,
> + },
> + {
> + "x86/cpuid/auto-xlevel/486/fixed/8000001A",
> + "486",
> + "xlevel=0x8000001A,3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> + NULL, "xlevel", 0x8000001A,
> + },
> + {
> + "x86/cpuid/auto-xlevel/phenom/fixed/0",
> + "486",
> + "xlevel=0,3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> + NULL, "xlevel", 0,
> + },
> + /* if xlevel2 is already large enough, it shouldn't change: */
> + {
> + "x86/cpuid/auto-xlevel2/486/fixed",
> + "486", "xlevel2=0xC0000002,xstore=on",
> + NULL, "xlevel2", 0xC0000002,
> + },
> +};
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> @@ -257,95 +389,13 @@ int main(int argc, char **argv)
> test_plus_minus_subprocess);
> g_test_add_func("/x86/cpuid/parsing-plus-minus", test_plus_minus);
>
> - /* Original level values for CPU models: */
> - add_cpuid_test("x86/cpuid/phenom/level",
> - "phenom", NULL, NULL, "level", 5);
> - add_cpuid_test("x86/cpuid/Conroe/level",
> - "Conroe", NULL, NULL, "level", 10);
> - add_cpuid_test("x86/cpuid/SandyBridge/level",
> - "SandyBridge", NULL, NULL, "level", 0xd);
> - add_cpuid_test("x86/cpuid/486/xlevel",
> - "486", NULL, NULL, "xlevel", 0);
> - add_cpuid_test("x86/cpuid/core2duo/xlevel",
> - "core2duo", NULL, NULL, "xlevel", 0x80000008);
> - add_cpuid_test("x86/cpuid/phenom/xlevel",
> - "phenom", NULL, NULL, "xlevel", 0x8000001A);
> - add_cpuid_test("x86/cpuid/athlon/xlevel",
> - "athlon", NULL, NULL, "xlevel", 0x80000008);
> -
> - /* If level is not large enough, it should increase automatically: */
> - /* CPUID[6].EAX: */
> - add_cpuid_test("x86/cpuid/auto-level/486/arat",
> - "486", "arat=on", NULL, "level", 6);
> - /* CPUID[EAX=7,ECX=0].EBX: */
> - add_cpuid_test("x86/cpuid/auto-level/phenom/fsgsbase",
> - "phenom", "fsgsbase=on", NULL, "level", 7);
> - /* CPUID[EAX=7,ECX=0].ECX: */
> - add_cpuid_test("x86/cpuid/auto-level/phenom/avx512vbmi",
> - "phenom", "avx512vbmi=on", NULL, "level", 7);
> - /* CPUID[EAX=0xd,ECX=1].EAX: */
> - add_cpuid_test("x86/cpuid/auto-level/phenom/xsaveopt",
> - "phenom", "xsaveopt=on", NULL, "level", 0xd);
> - /* CPUID[8000_0001].EDX: */
> - add_cpuid_test("x86/cpuid/auto-xlevel/486/3dnow",
> - "486", "3dnow=on", NULL, "xlevel", 0x80000001);
> - /* CPUID[8000_0001].ECX: */
> - add_cpuid_test("x86/cpuid/auto-xlevel/486/sse4a",
> - "486", "sse4a=on", NULL, "xlevel", 0x80000001);
> - /* CPUID[8000_0007].EDX: */
> - add_cpuid_test("x86/cpuid/auto-xlevel/486/invtsc",
> - "486", "invtsc=on", NULL, "xlevel", 0x80000007);
> - /* CPUID[8000_000A].EDX: */
> - add_cpuid_test("x86/cpuid/auto-xlevel/486/npt",
> - "486", "svm=on,npt=on", NULL, "xlevel", 0x8000000A);
> - /* CPUID[C000_0001].EDX: */
> - add_cpuid_test("x86/cpuid/auto-xlevel2/phenom/xstore",
> - "phenom", "xstore=on", NULL, "xlevel2", 0xC0000001);
> - /* SVM needs CPUID[0x8000000A] */
> - add_cpuid_test("x86/cpuid/auto-xlevel/athlon/svm",
> - "athlon", "svm=on", NULL, "xlevel", 0x8000000A);
> -
> -
> - /* If level is already large enough, it shouldn't change: */
> - add_cpuid_test("x86/cpuid/auto-level/SandyBridge/multiple",
> - "SandyBridge", "arat=on,fsgsbase=on,avx512vbmi=on",
> - NULL, "level", 0xd);
> - /* If level is explicitly set, it shouldn't change: */
> - add_cpuid_test("x86/cpuid/auto-level/486/fixed/0xF",
> - "486",
> - "level=0xF,arat=on,fsgsbase=on,avx512vbmi=on,xsaveopt=on",
> - NULL, "level", 0xF);
> - add_cpuid_test("x86/cpuid/auto-level/486/fixed/2",
> - "486",
> - "level=2,arat=on,fsgsbase=on,avx512vbmi=on,xsaveopt=on",
> - NULL, "level", 2);
> - add_cpuid_test("x86/cpuid/auto-level/486/fixed/0",
> - "486",
> - "level=0,arat=on,fsgsbase=on,avx512vbmi=on,xsaveopt=on",
> - NULL, "level", 0);
> -
> - /* if xlevel is already large enough, it shouldn't change: */
> - add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow",
> - "phenom", "3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> - NULL, "xlevel", 0x8000001A);
> - /* If xlevel is explicitly set, it shouldn't change: */
> - add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002",
> - "486",
> - "xlevel=0x80000002,3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> - NULL, "xlevel", 0x80000002);
> - add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A",
> - "486",
> - "xlevel=0x8000001A,3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> - NULL, "xlevel", 0x8000001A);
> - add_cpuid_test("x86/cpuid/auto-xlevel/phenom/fixed/0",
> - "486",
> - "xlevel=0,3dnow=on,sse4a=on,invtsc=on,npt=on,svm=on",
> - NULL, "xlevel", 0);
> -
> - /* if xlevel2 is already large enough, it shouldn't change: */
> - add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed",
> - "486", "xlevel2=0xC0000002,xstore=on",
> - NULL, "xlevel2", 0xC0000002);
> + for (int i = 0; i < ARRAY_SIZE(cpuid_tests); i++) {
> + if (!qtest_has_cpu_model(cpuid_tests[i].cpu)) {
> + continue;
> + }
> + qtest_add_data_func(cpuid_tests[i].name,
> + &cpuid_tests[i], test_cpuid_prop);
> + }
>
> /* Test feature parsing */
> add_feature_test("x86/cpuid/features/plus",
Reviewed-by: Fabiano Rosas <farosas@suse.de>
next prev parent reply other threads:[~2026-03-10 20:19 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-08 18:54 [PATCH 0/2] tests/qtest/test-x86-cpuid-compat: fix leaks Peter Maydell
2026-03-08 18:54 ` [PATCH 1/2] tests/qtest/test-x86-cpuid-compat: Switch test_cpuid_prop to data-driven Peter Maydell
2026-03-10 20:18 ` Fabiano Rosas [this message]
2026-03-08 18:54 ` [PATCH 2/2] tests/qtest/test-x86-cpuid-compat: Switch test_feature_flag " Peter Maydell
2026-03-10 20:18 ` Fabiano Rosas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tsunzbe8.fsf@suse.de \
--to=farosas@suse.de \
--cc=lvivier@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.