Re: [Qemu-devel] [PATCH v2] tests: Add [+-]feature and feature=on|off test cases

[Eduardo Habkost]

On Thu, May 11, 2017 at 04:00:00PM +0200, Igor Mammedov wrote:
> On Mon,  8 May 2017 15:32:05 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > Add test code to ensure features are enabled/disabled correctly in the
> > command-line. The test case use the "feature-words" and
> > "filtered-features" properties to check if the features were
> > enabled/disabled correctly.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> > * Remove explicit "accel=" option to avoid triggering warnings
> >   on "make check"
> > * Use qdict_get_*() helpers to make code shorter
> > * Rename input_eax, input_ecx to in_eax, in_ecx to make
> >   lines fit in the coding style width limit
> > * v1 was submitted as part of the series:
> >   Subject: [PATCH 0/4] x86: Support "-cpu feature=force"
> > * Coding style: split lines
> > * Style changes on code comments
> > ---
> >  tests/test-x86-cpuid-compat.c | 111 
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 111 insertions(+)
> > 
> > diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
> [...]
> > +    add_feature_test("x86/cpuid/features/max-plus-invtsc",
> > +                     "-cpu max,+invtsc",
> > +                     0x80000007, 0, "EDX", 8, true);
> > +    add_feature_test("x86/cpuid/features/max-invtsc-on",
> > +                     "-cpu max,invtsc=on",
> > +                     0x80000007, 0, "EDX", 8, true);
> > +    add_feature_test("x86/cpuid/features/max-minus-mmx",
> > +                     "-cpu max,-mmx",
> > +                     1, 0, "EDX", 23, false);
> > +    add_feature_test("x86/cpuid/features/max-invtsc-on,mmx=off",
> > +                     "-cpu max,mmx=off",
> > +                     1, 0, "EDX", 23, false);
> Why do you add 'max' variants in addition to 486/pentium?

Because we already had bugs in the host/max logic in the past.
See commit d4a606b38b5d4b3689b86cc1575908e82179ecfb for example.

Let's enumerate each test case and the reason for them:

+    add_feature_test("x86/cpuid/features/plus",
+                     "-cpu 486,+arat",
+                     6, 0, "EAX", 2, true);

Here I need a CPU model with level < 6. 486. pentium, pentium2,
pentium3 would work, and I chose 486. Conroe and newer won't
work, because they already have level=10.

+    add_feature_test("x86/cpuid/features/minus",
+                     "-cpu pentium,-mmx",
+                     1, 0, "EDX", 23, false);

Here I need a CPU model that has at least one feature I can
disable. I decided to test "-mmx", and use the oldest CPU model
that had MMX enabled (to increase the likelihood it is runnable
on the host).

+    add_feature_test("x86/cpuid/features/on",
+                     "-cpu 486,arat=on",
+                     6, 0, "EAX", 2, true);
+    add_feature_test("x86/cpuid/features/off",
+                     "-cpu pentium,mmx=off",
+                     1, 0, "EDX", 23, false);

Same as above, but testing feat=on|off syntax.

+    add_feature_test("x86/cpuid/features/max-plus-invtsc",
+                     "-cpu max,+invtsc",
+                     0x80000007, 0, "EDX", 8, true);
+    add_feature_test("x86/cpuid/features/max-invtsc-on",
+                     "-cpu max,invtsc=on",
+                     0x80000007, 0, "EDX", 8, true);

This is checking for the bugs fixed by commit
46c032f3afcc05a0123914609f1003906ba63fda and commit
d4a606b38b5d4b3689b86cc1575908e82179ecfb.


+    add_feature_test("x86/cpuid/features/max-minus-mmx",
+                     "-cpu max,-mmx",
+                     1, 0, "EDX", 23, false);
+    add_feature_test("x86/cpuid/features/max-invtsc-on,mmx=off",
+                     "-cpu max,mmx=off",
+                     1, 0, "EDX", 23, false);

This is checking the bug fixed by commit
d4a606b38b5d4b3689b86cc1575908e82179ecfb, but when disabling
features.


-- 
Eduardo