Re: [PATCH v3] KVM: selftests: Add a test for gPAT handling in L2

[Yosry Ahmed]

> > +             case UCALL_SYNC:
> > +                     if (do_save_restore) {
> > +                             pr_info("  Save/restore at sync point %ld\n",
> > +                                     uc.args[1]);
>
> Meh, this is noise to me.  Maybe it's marginally useful if there's a failure, but
> I don't see how it can possibly provide enough information to have to avoid
> hacking on the test.

I kept the logging in the test mostly as-is from the original patch, I
don't have a preference.

[..]
> > +             case UCALL_DONE:
> > +                     pr_info("  PASSED\n");
>
> This is useless and noisy.  The test runs in ~.25 seconds, and most of that is
> the printks.  If there's a failure, I'll see.  Otherwise it's a pretty safe
> assumption the test passed.

Same here.

[..]
> > +#define NPT_DISABLED 0
> > +#define NPT_ENABLED 1
> > +
> > +#define NO_SAVE_RESTORE 0
> > +#define DO_SAVE_RESTORE 1
>
> Eww.

I can't say that didn't hurt.

>
> > +
> > +#define NESTED_PAT_TEST(test_name, guest_code, npt, sr, iter)        \
> > +({                                                           \
>
> Unless you need to "return" a value, do-while (0) is preferred over ({}).  There's
> a technical reason, but I can't remember it off the top of my head.

Yeah I also couldn't remember it so I convinced myself that older
compilers just couldn't handle ({}).

Looking at https://kernelnewbies.org/FAQ/DoWhile0, the last section
seems to corroborate my story?

>
> > +     npt_enabled = npt;                                      \
> > +     nr_iterations = iter;                                   \
> > +     run_test(test_name, guest_code, sr);                    \
>
> LOL, all of this, and the one thing that _doesn't_ use macro shenanigans is
> string concatenation.
>
> > +})
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +     TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
> > +     TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_NPT));
> > +     TEST_REQUIRE(kvm_has_cap(KVM_CAP_NESTED_STATE));
> > +     TEST_REQUIRE(kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) &
> > +                  KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> > +
> > +     NESTED_PAT_TEST("Invalid gPAT", l1_guest_code_invalid_gpat,
> > +                     NPT_ENABLED, NO_SAVE_RESTORE, 1);
> > +
> > +     NESTED_PAT_TEST("Nested NPT disabled", l1_guest_code,
> > +                     NPT_DISABLED, NO_SAVE_RESTORE, 1);
> > +
> > +     NESTED_PAT_TEST("Nested NPT enabled", l1_guest_code,
> > +                     NPT_ENABLED, NO_SAVE_RESTORE, 1);
> > +
> > +     NESTED_PAT_TEST("Save/Restore", l1_guest_code,
> > +                     NPT_ENABLED, DO_SAVE_RESTORE, 1);
> > +
> > +     NESTED_PAT_TEST("Multiple VM-Entries", l1_guest_code,
> > +                     NPT_ENABLED, NO_SAVE_RESTORE, 10);
>
> I appreciated trying to avoid true/false literals, but this is harder to read.
> At least "true" and "false" are visually different, {N,D}O_SAVE_RESTORE are one
> char, and NPT_{DIS,EN}ABLED are two.

That's fair.

>
> And explicitly defining the number of iterations and whether or not to do save/restore
> is silly.  It requires _more_ code to provide _less_ coverage.  Just run the combos
> for npt_enabled=false.

Also fair.

>
> If we're going to use macro shenanigans, let's actually use them!  I've got this
> working, I'll post a v4.
>
> #define gpat_test(test_name, guest_code, npt_input)                     \
> do {                                                                    \
>         npt_input;                                                      \

Oh I like this very much. Passing in "npt = true" or "npt = false" is
great. I wouldn't call it npt_input (maybe npt_setting or sth), but
that doesn't matter much.

>                                                                         \
>         pr_info("Testing: " test_name "\n");                            \
>         run_test(guest_code, false, 1);                                 \
>                                                                         \
>         if (guest_code == l1_guest_code) {                              \

I thought about doing this, but I was worried about it breaking if we
add more L1 guest code functions. In retrospect that was too paranoid
(as usual).

>                 pr_info("Testing: " test_name " Save/Restore\n");       \
>                 run_test(guest_code, true, 1);                          \
>                                                                         \
>                 pr_info("Testing: " test_name " Multiple VMRUNs\n");    \
>                 run_test(guest_code, false, 10);                        \
>         }                                                               \
> } while (0)
>
> int main(int argc, char *argv[])
> {
>         TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
>         TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_NPT));
>         TEST_REQUIRE(kvm_has_cap(KVM_CAP_NESTED_STATE));
>         TEST_REQUIRE(kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) &
>                      KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
>
>         gpat_test("Invalid gPAT", l1_guest_code_invalid_gpat, npt_enabled = true);
>         gpat_test("Nested NPT disabled", l1_guest_code, npt_enabled = false);
>         gpat_test("Nested NPT enabled", l1_guest_code, npt_enabled = true);

Overall looks great, thanks Sean.