[kvm-unit-tests PATCH 0/3] nSVM: Add testing for routing L2 exceptions

[kvm-unit-tests PATCH 0/3] nSVM: Add testing for routing L2 exceptions

[Manali Shukla]

Series is inspired by vmx exception test framework series[1].

Set up a test framework that verifies an exception occurring in L2 is 
forwarded to the right place (L1 or L2).

Tests two conditions for each exception.
1) Exception generated in L2, is handled by L2 when L2 exception handler
   is registered.
2) Exception generated in L2, is handled by L1 when intercept exception
   bit map is set in L1.

Above tests were added to verify 8 different exceptions
#GP, #UD, #DE, #BP, #NM, #OF, #DB, #AC.

There are 3 patches in this series
1) Added routines to set/clear PT_USER_MASK to make #AC test work for nSVM. 
2) exception_mnemonic patch is taken from the Aaron's vmx series[1]. 
3) Added test infrastructure and exception tests.

[1] https://lore.kernel.org/all/20220125203127.1161838-1-aaronlewis@google.com/
Aaron Lewis (1):
  x86: Make exception_mnemonic() visible to the tests

Manali Shukla (2):
  x86: Add routines to set/clear PT_USER_MASK for all pages
  x86: nSVM: Add an exception test framework and tests

 lib/x86/desc.c  |   2 +-
 lib/x86/desc.h  |   1 +
 lib/x86/vm.c    |  54 ++++++++++++++
 lib/x86/vm.h    |   3 +
 x86/svm_tests.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 244 insertions(+), 1 deletion(-)

-- 
2.30.2

[kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Manali Shukla]

Add following 2 routines :
1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables

commit 916635a813e975600335c6c47250881b7a328971
(nSVM: Add test for NPT reserved bit and #NPF error code behavior)
clears PT_USER_MASK for all svm testcases. Any tests that requires
usermode access will fail after this commit.

Usermode function needs to be called from L2 guest to generate
AC exception and calling usermode function with supervisor pages
generates #PF exception with error code 0004

Add solution to above mentioned problem which is to set
PT_USER_MASK for all pages in the initialization of #AC exception
test and clear them in uninitialization of #AC exception at last.
AC exception test works fine without hampering other test cases with
this solution.

Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 lib/x86/vm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/x86/vm.h |  3 +++
 2 files changed, 57 insertions(+)

diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 56be57b..f3a7ae8 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -37,6 +37,60 @@ pteval_t *install_pte(pgd_t *cr3,
     return &pt[offset];
 }
 
+/*
+ * set PT_USER_MASK bit for all levels of page tables
+ */
+
+void set_user_mask_all(pteval_t *pt, int level)
+{
+    pteval_t pte, *ptep;
+    int i;
+
+    if (level == PAGE_LEVEL)
+        pte_opt_mask |= PT_USER_MASK;
+
+    for (i = 0; i < 512; i++) {
+        ptep = &pt[i];
+        pte = *ptep;
+
+        if ((pte & PT_PRESENT_MASK) && !(pte & PT_USER_MASK)) {
+            *ptep |= PT_USER_MASK;
+
+            if (level == 1 || pte & PT_PAGE_SIZE_MASK)
+                continue;
+
+            set_user_mask_all(phys_to_virt(pte & 0xffffffffff000ull), level - 1);
+        }
+    }
+}
+
+
+/*
+ * clear PT_USER_MASK bit for all levels of page tables
+ */
+
+void clear_user_mask_all(pteval_t *pt, int level)
+{
+    pteval_t pte, *ptep;
+    int i;
+
+    for (i = 0; i < 512; i++) {
+        ptep = &pt[i];
+        pte = *ptep;
+
+        if ((pte & PT_PRESENT_MASK) && (pte & PT_USER_MASK)) {
+            *ptep &= ~PT_USER_MASK;
+
+            if (level == 1 || pte & PT_PAGE_SIZE_MASK)
+                continue;
+
+            clear_user_mask_all(phys_to_virt(pte & 0xffffffffff000ull), level - 1);
+        }
+    }
+    if (level == PAGE_LEVEL)
+        pte_opt_mask &= ~PT_USER_MASK;
+}
+
 /*
  * Finds last PTE in the mapping of @virt that's at or above @lowest_level. The
  * returned PTE isn't necessarily present, but its parent is.
diff --git a/lib/x86/vm.h b/lib/x86/vm.h
index 4c6dff9..75715e5 100644
--- a/lib/x86/vm.h
+++ b/lib/x86/vm.h
@@ -38,6 +38,9 @@ pteval_t *install_large_page(pgd_t *cr3, phys_addr_t phys, void *virt);
 void install_pages(pgd_t *cr3, phys_addr_t phys, size_t len, void *virt);
 bool any_present_pages(pgd_t *cr3, void *virt, size_t len);
 
+void set_user_mask_all(pteval_t *pt, int level);
+void clear_user_mask_all(pteval_t *pt, int level);
+
 static inline void *current_page_table(void)
 {
 	return phys_to_virt(read_cr3());
-- 
2.30.2

[kvm-unit-tests PATCH 2/3] x86: Make exception_mnemonic() visible to the tests

[Manali Shukla]

From: Aaron Lewis <aaronlewis@google.com>

exception_mnemonic() is a useful function for more than just desc.c.
Make it global, so it can be used in other KUT tests.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 lib/x86/desc.c | 2 +-
 lib/x86/desc.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 16b7256..c2eb16e 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -91,7 +91,7 @@ struct ex_record {
 
 extern struct ex_record exception_table_start, exception_table_end;
 
-static const char* exception_mnemonic(int vector)
+const char* exception_mnemonic(int vector)
 {
 	switch(vector) {
 	case 0: return "#DE";
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 9b81da0..ad6277b 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -224,6 +224,7 @@ void set_intr_alt_stack(int e, void *fn);
 void print_current_tss_info(void);
 handler handle_exception(u8 v, handler fn);
 void unhandled_exception(struct ex_regs *regs, bool cpu);
+const char* exception_mnemonic(int vector);
 
 bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
 			void *data);
-- 
2.30.2

[kvm-unit-tests PATCH 3/3] x86: nSVM: Add an exception test framework and tests

[Manali Shukla]

Set up a test framework that verifies an exception occurring
in L2 is forwarded to the right place (L1 or L2)
It adds an exception test array and exception callbacks to that array.

Tests two conditions for each exception
1) Exception generated in L2, is handled by L2 when L2 exception handler
   is registered.
2) Exception generated in L2, is handled by L1 when intercept exception
   bit map is set in L1.

Add testing for below exceptions:
(#GP, #UD, #DE, #BP, #NM, #OF, #DB, #AC)
1. #GP is generated in c by non-canonical access in L2.
2. #UD is generated by calling "ud2" instruction in L2.
3. #DE is generated using instrumented code which generates
   divide by zero condition
4. #BP is generated by calling "int3" instruction in L2.
5. #NM is generated by calling floating point instruction "fnop"
   in L2 when TS bit is set.
6. #OF is generated using instrumented code and "into" instruction
   is called in that code in L2.
7. #DB is generated by setting TF bit before entering to L2.
8. #AC is genrated by writing 8 bytes to 4 byte aligned address in L2
   user mode when AM bit is set in CR0 register and AC bit is set in
   RFLAGS

Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 x86/svm_tests.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 185 insertions(+)

diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 0707786..66bfb51 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -10,6 +10,7 @@
 #include "isr.h"
 #include "apic.h"
 #include "delay.h"
+#include "x86/usermode.h"
 
 #define SVM_EXIT_MAX_DR_INTERCEPT 0x3f
 
@@ -3074,6 +3075,189 @@ static void svm_nm_test(void)
         "fnop with CR0.TS and CR0.EM unset no #NM excpetion");
 }
 
+static void svm_l2_gp_test(struct svm_test *test)
+{
+    *(volatile u64 *)NONCANONICAL = 0;
+}
+
+static void svm_l2_ud_test(struct svm_test *test)
+{
+    asm volatile ("ud2");
+}
+
+static void svm_l2_de_test(struct svm_test *test)
+{
+        asm volatile (
+                "xor %%eax, %%eax\n\t"
+                "xor %%ebx, %%ebx\n\t"
+                "xor %%edx, %%edx\n\t"
+                "idiv %%ebx\n\t"
+                ::: "eax", "ebx", "edx");
+}
+
+static void svm_l2_bp_test(struct svm_test *svm)
+{
+    asm volatile ("int3");
+}
+
+static void svm_l2_nm_test(struct svm_test *svm)
+{
+    write_cr0(read_cr0() | X86_CR0_TS);
+    asm volatile("fnop");
+}
+
+static void svm_l2_of_test(struct svm_test *svm)
+{
+    struct far_pointer32 fp = {
+        .offset = (uintptr_t)&&into,
+        .selector = KERNEL_CS32,
+    };
+    uintptr_t rsp;
+
+    asm volatile ("mov %%rsp, %0" : "=r"(rsp));
+
+    if (fp.offset != (uintptr_t)&&into) {
+        printf("Codee address too high.\n");
+        return;
+    }
+
+    if ((u32)rsp != rsp) {
+        printf("Stack address too high.\n");
+    }
+
+    asm goto("lcall *%0" : : "m" (fp) : "rax" : into);
+    return;
+into:
+    asm volatile (".code32;"
+            "movl $0x7fffffff, %eax;"
+            "addl %eax, %eax;"
+            "into;"
+            "lret;"
+            ".code64");
+    __builtin_unreachable();
+}
+
+static void svm_l2_db_test(struct svm_test *test)
+{
+    write_rflags(read_rflags() | X86_EFLAGS_TF);
+}
+
+static uint64_t usermode_callback(void)
+{
+   /*
+    * Trigger an #AC by writing 8 bytes to a 4-byte aligned address.
+    * Disclaimer: It is assumed that the stack pointer is aligned
+    * on a 16-byte boundary as x86_64 stacks should be.
+    */
+    asm volatile("movq $0, -0x4(%rsp)");
+
+    return 0;
+}
+
+static void svm_l2_ac_test(struct svm_test *test)
+{
+    bool hit_ac = false;
+
+    write_cr0(read_cr0() | X86_CR0_AM);
+    write_rflags(read_rflags() | X86_EFLAGS_AC);
+ 
+    run_in_user(usermode_callback, AC_VECTOR, 0, 0, 0, 0, &hit_ac);
+
+    report(hit_ac, "Usermode #AC handled in L2");
+    vmmcall();
+}
+
+static void svm_ac_init(void)
+{
+    set_user_mask_all(phys_to_virt(read_cr3()), PAGE_LEVEL);
+}
+
+static void svm_ac_uninit(void)
+{
+    clear_user_mask_all(phys_to_virt(read_cr3()), PAGE_LEVEL);
+}
+
+struct svm_exception_test {
+    u8 vector;
+    void (*guest_code)(struct svm_test*);
+    void (*init_test)(void);
+    void (*uninit_test)(void);
+};
+
+struct svm_exception_test svm_exception_tests[] = {
+    { GP_VECTOR, svm_l2_gp_test },
+    { UD_VECTOR, svm_l2_ud_test },
+    { DE_VECTOR, svm_l2_de_test },
+    { BP_VECTOR, svm_l2_bp_test },
+    { NM_VECTOR, svm_l2_nm_test },
+    { OF_VECTOR, svm_l2_of_test },
+    { DB_VECTOR, svm_l2_db_test },
+    { AC_VECTOR, svm_l2_ac_test, svm_ac_init, svm_ac_uninit },
+};
+
+static u8 svm_exception_test_vector;
+
+static void svm_exception_handler(struct ex_regs *regs)
+{
+    report(regs->vector == svm_exception_test_vector,
+            "Handling %s in L2's exception handler",
+            exception_mnemonic(svm_exception_test_vector));
+    vmmcall();
+}
+
+static void handle_exception_in_l2(u8 vector)
+{
+    handler old_handler = handle_exception(vector, svm_exception_handler);
+    svm_exception_test_vector = vector;
+
+    report(svm_vmrun() == SVM_EXIT_VMMCALL,
+           "%s handled by L2", exception_mnemonic(vector));
+
+    handle_exception(vector, old_handler);
+}
+
+static void handle_exception_in_l1(u32 vector)
+{
+    u32 old_ie = vmcb->control.intercept_exceptions;
+
+    vmcb->control.intercept_exceptions |= (1ULL << vector);
+
+    report(svm_vmrun() == (SVM_EXIT_EXCP_BASE + vector),
+           "%s handled by L1",  exception_mnemonic(vector));
+
+    vmcb->control.intercept_exceptions = old_ie;
+}
+
+static void svm_exception_test(void)
+{
+    struct svm_exception_test *t;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(svm_exception_tests); i++) {
+        t = &svm_exception_tests[i];
+        test_set_guest(t->guest_code);
+        if (t->init_test)
+            t->init_test();
+
+        handle_exception_in_l2(t->vector);
+
+        if (t->uninit_test)
+            t->uninit_test();
+
+        vmcb_ident(vmcb);
+
+        if (t->init_test)
+            t->init_test();
+
+        handle_exception_in_l1(t->vector);
+
+        if (t->uninit_test)
+            t->uninit_test();
+
+        vmcb_ident(vmcb);
+    }
+}
+
 struct svm_test svm_tests[] = {
     { "null", default_supported, default_prepare,
       default_prepare_gif_clear, null_test,
@@ -3196,5 +3380,6 @@ struct svm_test svm_tests[] = {
     TEST(svm_nm_test),
     TEST(svm_int3_test),
     TEST(svm_into_test),
+    TEST(svm_exception_test),
     { NULL, NULL, NULL, NULL, NULL, NULL, NULL }
 };
-- 
2.30.2

Re: [kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Sean Christopherson]

On Mon, Feb 07, 2022, Manali Shukla wrote:
> Add following 2 routines :
> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables
> 
> commit 916635a813e975600335c6c47250881b7a328971
> (nSVM: Add test for NPT reserved bit and #NPF error code behavior)
> clears PT_USER_MASK for all svm testcases. Any tests that requires
> usermode access will fail after this commit.

Gah, I took the easy route and it burned us.  I would rather we start breaking up
the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag
only in that test, not the "common" nSVM test.

If we don't do that, this should really use walk_pte() instead of adding yet
another PTE walker.

Re: [kvm-unit-tests PATCH 3/3] x86: nSVM: Add an exception test framework and tests

[Aaron Lewis]

> +static void svm_l2_nm_test(struct svm_test *svm)
> +{
> +    write_cr0(read_cr0() | X86_CR0_TS);
> +    asm volatile("fnop");
> +}
> +
> +static void svm_l2_of_test(struct svm_test *svm)
> +{
> +    struct far_pointer32 fp = {
> +        .offset = (uintptr_t)&&into,
> +        .selector = KERNEL_CS32,
> +    };
> +    uintptr_t rsp;
> +
> +    asm volatile ("mov %%rsp, %0" : "=r"(rsp));
> +
> +    if (fp.offset != (uintptr_t)&&into) {
> +        printf("Codee address too high.\n");

Nit: Code

> +        return;
> +    }
> +
> +    if ((u32)rsp != rsp) {
> +        printf("Stack address too high.\n");
> +    }
> +
> +    asm goto("lcall *%0" : : "m" (fp) : "rax" : into);
> +    return;
> +into:
> +    asm volatile (".code32;"
> +            "movl $0x7fffffff, %eax;"
> +            "addl %eax, %eax;"
> +            "into;"
> +            "lret;"
> +            ".code64");
> +    __builtin_unreachable();
> +}
> +

> +static void svm_l2_ac_test(struct svm_test *test)
> +{
> +    bool hit_ac = false;
> +
> +    write_cr0(read_cr0() | X86_CR0_AM);
> +    write_rflags(read_rflags() | X86_EFLAGS_AC);
> +
> +    run_in_user(usermode_callback, AC_VECTOR, 0, 0, 0, 0, &hit_ac);
> +
> +    report(hit_ac, "Usermode #AC handled in L2");
> +    vmmcall();
> +}
> +
> +static void svm_ac_init(void)
> +{
> +    set_user_mask_all(phys_to_virt(read_cr3()), PAGE_LEVEL);
> +}
> +
> +static void svm_ac_uninit(void)
> +{
> +    clear_user_mask_all(phys_to_virt(read_cr3()), PAGE_LEVEL);
> +}
> +
> +struct svm_exception_test {
> +    u8 vector;
> +    void (*guest_code)(struct svm_test*);
> +    void (*init_test)(void);
> +    void (*uninit_test)(void);
> +};
> +
> +struct svm_exception_test svm_exception_tests[] = {
> +    { GP_VECTOR, svm_l2_gp_test },
> +    { UD_VECTOR, svm_l2_ud_test },
> +    { DE_VECTOR, svm_l2_de_test },
> +    { BP_VECTOR, svm_l2_bp_test },
> +    { NM_VECTOR, svm_l2_nm_test },
> +    { OF_VECTOR, svm_l2_of_test },
> +    { DB_VECTOR, svm_l2_db_test },
> +    { AC_VECTOR, svm_l2_ac_test, svm_ac_init, svm_ac_uninit },
> +};

If you set and clear PT_USER_MASK in svm_l2_ac_test() before calling
into userspace you can remove init_test and uninit_test from the
framework all together.  That will simplify the code.

Further, it would be nice to then hoist this framework and the one in
vmx into a common x86 file, but looking at this that may be something
to think about in the future.  There would have to be wrappers when
interacting with the vmc{s,b} and macros at the very least.

> +
> +static u8 svm_exception_test_vector;
> +
> +static void svm_exception_handler(struct ex_regs *regs)
> +{
> +    report(regs->vector == svm_exception_test_vector,
> +            "Handling %s in L2's exception handler",
> +            exception_mnemonic(svm_exception_test_vector));
> +    vmmcall();
> +}
> +

Re: [kvm-unit-tests PATCH 3/3] x86: nSVM: Add an exception test framework and tests

[Shukla, Manali]

On 2/15/2022 1:50 AM, Aaron Lewis wrote:
>> +static void svm_l2_nm_test(struct svm_test *svm)
>> +{
>> +    write_cr0(read_cr0() | X86_CR0_TS);
>> +    asm volatile("fnop");
>> +}
>> +
>> +static void svm_l2_of_test(struct svm_test *svm)
>> +{
>> +    struct far_pointer32 fp = {
>> +        .offset = (uintptr_t)&&into,
>> +        .selector = KERNEL_CS32,
>> +    };
>> +    uintptr_t rsp;
>> +
>> +    asm volatile ("mov %%rsp, %0" : "=r"(rsp));
>> +
>> +    if (fp.offset != (uintptr_t)&&into) {
>> +        printf("Codee address too high.\n");
> 
> Nit: Code
> 
>> +        return;
>> +    }
>> +
>> +    if ((u32)rsp != rsp) {
>> +        printf("Stack address too high.\n");
>> +    }
>> +
>> +    asm goto("lcall *%0" : : "m" (fp) : "rax" : into);
>> +    return;
>> +into:
>> +    asm volatile (".code32;"
>> +            "movl $0x7fffffff, %eax;"
>> +            "addl %eax, %eax;"
>> +            "into;"
>> +            "lret;"
>> +            ".code64");
>> +    __builtin_unreachable();
>> +}
>> +
> 
>> +static void svm_l2_ac_test(struct svm_test *test)
>> +{
>> +    bool hit_ac = false;
>> +
>> +    write_cr0(read_cr0() | X86_CR0_AM);
>> +    write_rflags(read_rflags() | X86_EFLAGS_AC);
>> +
>> +    run_in_user(usermode_callback, AC_VECTOR, 0, 0, 0, 0, &hit_ac);
>> +
>> +    report(hit_ac, "Usermode #AC handled in L2");
>> +    vmmcall();
>> +}
>> +
>> +static void svm_ac_init(void)
>> +{
>> +    set_user_mask_all(phys_to_virt(read_cr3()), PAGE_LEVEL);
>> +}
>> +
>> +static void svm_ac_uninit(void)
>> +{
>> +    clear_user_mask_all(phys_to_virt(read_cr3()), PAGE_LEVEL);
>> +}
>> +
>> +struct svm_exception_test {
>> +    u8 vector;
>> +    void (*guest_code)(struct svm_test*);
>> +    void (*init_test)(void);
>> +    void (*uninit_test)(void);
>> +};
>> +
>> +struct svm_exception_test svm_exception_tests[] = {
>> +    { GP_VECTOR, svm_l2_gp_test },
>> +    { UD_VECTOR, svm_l2_ud_test },
>> +    { DE_VECTOR, svm_l2_de_test },
>> +    { BP_VECTOR, svm_l2_bp_test },
>> +    { NM_VECTOR, svm_l2_nm_test },
>> +    { OF_VECTOR, svm_l2_of_test },
>> +    { DB_VECTOR, svm_l2_db_test },
>> +    { AC_VECTOR, svm_l2_ac_test, svm_ac_init, svm_ac_uninit },
>> +};
> 
> If you set and clear PT_USER_MASK in svm_l2_ac_test() before calling
> into userspace you can remove init_test and uninit_test from the
> framework all together.  That will simplify the code.
> 
If clear user mask is called after userspace code, when #AC exception is 
intercepted by L1, the control directly goes to L1 and it does not reach 
clear_user_mask_all() function (called after user space code function run_in_user()).

That is why I have added init_test and uninit_test function

> Further, it would be nice to then hoist this framework and the one in
> vmx into a common x86 file, but looking at this that may be something
> to think about in the future.  There would have to be wrappers when
> interacting with the vmc{s,b} and macros at the very least.
> 

Yeah we can think of this in future.

>> +
>> +static u8 svm_exception_test_vector;
>> +
>> +static void svm_exception_handler(struct ex_regs *regs)
>> +{
>> +    report(regs->vector == svm_exception_test_vector,
>> +            "Handling %s in L2's exception handler",
>> +            exception_mnemonic(svm_exception_test_vector));
>> +    vmmcall();
>> +}
>> +

-Manali

Re: [kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Shukla, Manali]

On 2/15/2022 1:00 AM, Sean Christopherson wrote:
> On Mon, Feb 07, 2022, Manali Shukla wrote:
>> Add following 2 routines :
>> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
>> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables
>>
>> commit 916635a813e975600335c6c47250881b7a328971
>> (nSVM: Add test for NPT reserved bit and #NPF error code behavior)
>> clears PT_USER_MASK for all svm testcases. Any tests that requires
>> usermode access will fail after this commit.
> 
> Gah, I took the easy route and it burned us.  I would rather we start breaking up
> the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag
> only in that test, not the "common" nSVM test.

Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and 
set User flag by default for all the test cases by calling setup_vm()
and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test().

Walk_pte() routine uses start address and length to walk over the memory 
region where flag needs to be set/clear. So start address and length need to be
figured out to use this routine.

I will work on this.

> 
> If we don't do that, this should really use walk_pte() instead of adding yet
> another PTE walker.

-Manali

Re: [kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Aaron Lewis]

On Thu, Feb 17, 2022 at 3:55 AM Shukla, Manali <mashukla@amd.com> wrote:
>
>
>
> On 2/15/2022 1:00 AM, Sean Christopherson wrote:
> > On Mon, Feb 07, 2022, Manali Shukla wrote:
> >> Add following 2 routines :
> >> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
> >> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables
> >>
> >> commit 916635a813e975600335c6c47250881b7a328971
> >> (nSVM: Add test for NPT reserved bit and #NPF error code behavior)
> >> clears PT_USER_MASK for all svm testcases. Any tests that requires
> >> usermode access will fail after this commit.
> >
> > Gah, I took the easy route and it burned us.  I would rather we start breaking up
> > the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag
> > only in that test, not the "common" nSVM test.
>
> Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and
> set User flag by default for all the test cases by calling setup_vm()
> and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test().
>
> Walk_pte() routine uses start address and length to walk over the memory
> region where flag needs to be set/clear. So start address and length need to be
> figured out to use this routine.

For the start address and length you can use 'stext' and 'etext' to
compute those.  There's an example in access.c set_cr4_smep(),
however, it uses walk_ptes() as opposed to walk_pte() (similar, but
different).

>
> I will work on this.
>
> >
> > If we don't do that, this should really use walk_pte() instead of adding yet
> > another PTE walker.
>
> -Manali

Re: [kvm-unit-tests PATCH 3/3] x86: nSVM: Add an exception test framework and tests

[Aaron Lewis]

> >> +};
> >
> > If you set and clear PT_USER_MASK in svm_l2_ac_test() before calling
> > into userspace you can remove init_test and uninit_test from the
> > framework all together.  That will simplify the code.
> >
> If clear user mask is called after userspace code, when #AC exception is
> intercepted by L1, the control directly goes to L1 and it does not reach
> clear_user_mask_all() function (called after user space code function run_in_user()).
>
> That is why I have added init_test and uninit_test function
>

Ah, that makes sense.  Though IIUC, you are now moving the set/clear
elsewhere, right?  If so, it seems like init_test() and uninit_test()
are no longer needed.

> > Further, it would be nice to then hoist this framework and the one in
> > vmx into a common x86 file, but looking at this that may be something
> > to think about in the future.  There would have to be wrappers when
> > interacting with the vmc{s,b} and macros at the very least.
> >

Re: [kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Sean Christopherson]

On Thu, Feb 17, 2022, Shukla, Manali wrote:
> 
> On 2/15/2022 1:00 AM, Sean Christopherson wrote:
> > On Mon, Feb 07, 2022, Manali Shukla wrote:
> >> Add following 2 routines :
> >> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
> >> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables
> >>
> >> commit 916635a813e975600335c6c47250881b7a328971
> >> (nSVM: Add test for NPT reserved bit and #NPF error code behavior)
> >> clears PT_USER_MASK for all svm testcases. Any tests that requires
> >> usermode access will fail after this commit.
> > 
> > Gah, I took the easy route and it burned us.  I would rather we start breaking up
> > the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag
> > only in that test, not the "common" nSVM test.
> 
> Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and 
> set User flag by default for all the test cases by calling setup_vm()
> and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test().

I was thinking of something more drastic.  The only reason the nSVM tests are
"incompatible" with usermode is this snippet in main():

  int main(int ac, char **av)
  {
	/* Omit PT_USER_MASK to allow tested host.CR4.SMEP=1. */
	pteval_t opt_mask = 0;
	int i = 0;

	ac--;
	av++;

	__setup_vm(&opt_mask);

	...
  }

Change that to setup_vm() and KUT will build the test with PT_USER_MASK set on
all PTEs.  My thought (might be a bad one) is to move the nNPT tests to their own
file/test so that the tests don't need to fiddle with page tables midway through.

The quick and dirty approach would be to turn the current main() into a small
helper, minus its call to __setup_vm().

Longer term, I think it'd make sense to add svm/ +  vmx/ subdirectories, and turn
much of the common code into proper libraries, e.g. test_wanted() can and should
be common helper, probably with more glue code to allow declaring a set of subtests.
But for now I think we can just add svm_npt.c or whatever.

Re: [kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Shukla, Manali]

On 2/17/2022 8:04 PM, Aaron Lewis wrote:
> On Thu, Feb 17, 2022 at 3:55 AM Shukla, Manali <mashukla@amd.com> wrote:
>>
>>
>>
>> On 2/15/2022 1:00 AM, Sean Christopherson wrote:
>>> On Mon, Feb 07, 2022, Manali Shukla wrote:
>>>> Add following 2 routines :
>>>> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
>>>> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables
>>>>
>>>> commit 916635a813e975600335c6c47250881b7a328971
>>>> (nSVM: Add test for NPT reserved bit and #NPF error code behavior)
>>>> clears PT_USER_MASK for all svm testcases. Any tests that requires
>>>> usermode access will fail after this commit.
>>>
>>> Gah, I took the easy route and it burned us.  I would rather we start breaking up
>>> the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag
>>> only in that test, not the "common" nSVM test.
>>
>> Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and
>> set User flag by default for all the test cases by calling setup_vm()
>> and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test().
>>
>> Walk_pte() routine uses start address and length to walk over the memory
>> region where flag needs to be set/clear. So start address and length need to be
>> figured out to use this routine.
> 
> For the start address and length you can use 'stext' and 'etext' to
> compute those.  There's an example in access.c set_cr4_smep(),
> however, it uses walk_ptes() as opposed to walk_pte() (similar, but
> different).
> 
Yeah PT_USER_MASK is only set for text segment in set_cr4_smep(). 
In #AC exception test, PT_USER_MASK needs to be set for user stack pages.
So, setting PT_USER_MASK for length('etext' - 'stext') of 
memory will not resolve the problem. 
Please let me know if I am missing something. 

>>
>> I will work on this.
>>
>>>
>>> If we don't do that, this should really use walk_pte() instead of adding yet
>>> another PTE walker.
>>
>> -Manali

Re: [kvm-unit-tests PATCH 1/3] x86: Add routines to set/clear PT_USER_MASK for all pages

[Shukla, Manali]

On 2/17/2022 8:50 PM, Sean Christopherson wrote:
> On Thu, Feb 17, 2022, Shukla, Manali wrote:
>>
>> On 2/15/2022 1:00 AM, Sean Christopherson wrote:
>>> On Mon, Feb 07, 2022, Manali Shukla wrote:
>>>> Add following 2 routines :
>>>> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables
>>>> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables
>>>>
>>>> commit 916635a813e975600335c6c47250881b7a328971
>>>> (nSVM: Add test for NPT reserved bit and #NPF error code behavior)
>>>> clears PT_USER_MASK for all svm testcases. Any tests that requires
>>>> usermode access will fail after this commit.
>>>
>>> Gah, I took the easy route and it burned us.  I would rather we start breaking up
>>> the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag
>>> only in that test, not the "common" nSVM test.
>>
>> Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and 
>> set User flag by default for all the test cases by calling setup_vm()
>> and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test().
> 
> I was thinking of something more drastic.  The only reason the nSVM tests are
> "incompatible" with usermode is this snippet in main():
> 
>   int main(int ac, char **av)
>   {
> 	/* Omit PT_USER_MASK to allow tested host.CR4.SMEP=1. */
> 	pteval_t opt_mask = 0;
> 	int i = 0;
> 
> 	ac--;
> 	av++;
> 
> 	__setup_vm(&opt_mask);
> 
> 	...
>   }
> 
> Change that to setup_vm() and KUT will build the test with PT_USER_MASK set on
> all PTEs.  My thought (might be a bad one) is to move the nNPT tests to their own
> file/test so that the tests don't need to fiddle with page tables midway through.
> 
> The quick and dirty approach would be to turn the current main() into a small
> helper, minus its call to __setup_vm().

Yeah this seems to be a better idea. Based on your suggestions, I am planning to do 
following.
	1) Move svm_npt_rsvd_bits_test() to file svm_npt.c and run it with __setup_vm()
	2) There are 7 more npt test cases in svm_tests.c, move them to svm_npt.c 
	   Below are the test cases:
           npt_nx, npt_np, npt_us, npt_rw, npt_rw_pfwalk, npt_l1mmio, npt_rw_l1mmio
 	3) Change __setup_vm() to setup_vm() in svm_tests.c ( after doing this #AC 
	   exception test will work fine without the need of set_user_mask_all and
           clear_user_mask_all)
> 
> Longer term, I think it'd make sense to add svm/ +  vmx/ subdirectories, and turn
> much of the common code into proper libraries, e.g. test_wanted() can and should
> be common helper, probably with more glue code to allow declaring a set of subtests.
> But for now I think we can just add svm_npt.c or whatever.
I agree, we can do something similar in future.