[PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm

[PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm

[Lara Lazier]

Following the APM2 I added some checks to 
resolve the following tests in kvm-unit-tests for svm:

  * vmrun_intercept_check
  * asid_zero
  * sel_cr0_bug
  * CR0 CD=0,NW=1: a0010011
  * CR0 63:32: 180010011
  * CR0 63:32: 1080010011
  * CR0 63:32: 10080010011
  * CR0 63:32: 100080010011
  * CR0 63:32: 1000080010011
  * CR0 63:32: 10000080010011
  * CR0 63:32: 100000080010011
  * CR0 63:32: 1000000080010011

v1->v2: introduced cpu_svm_has_intercept to avoid defining bitmasks for 
        intercepts

Lara Lazier (4):
  target/i386: Refactored intercept checks into cpu_svm_has_intercept
  target/i386: Added consistency checks for VMRUN intercept and ASID
  target/i386: Added consistency checks for CR0
  target/i386: Added Intercept CR0 writes check

 target/i386/cpu.h                    |   5 ++
 target/i386/svm.h                    |   2 +
 target/i386/tcg/sysemu/misc_helper.c |   9 ++
 target/i386/tcg/sysemu/svm_helper.c  | 127 ++++++++++++++++-----------
 4 files changed, 93 insertions(+), 50 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] target/i386: Refactored intercept checks into cpu_svm_has_intercept

[Lara Lazier]

Added cpu_svm_has_intercept to reduce duplication when checking the
corresponding intercept bit outside of cpu_svm_check_intercept_param

Signed-off-by: Lara Lazier <laramglazier@gmail.com>
---
 target/i386/cpu.h                   |   3 +
 target/i386/tcg/sysemu/svm_helper.c | 105 +++++++++++++++-------------
 2 files changed, 61 insertions(+), 47 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ac3abea97c..59b9231ee2 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2149,9 +2149,12 @@ static inline void
 cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
                               uint64_t param, uintptr_t retaddr)
 { /* no-op */ }
+bool cpu_svm_has_intercept(CPUX86State *env, uint32_t type)
+{ return false; }
 #else
 void cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
                                    uint64_t param, uintptr_t retaddr);
+bool cpu_svm_has_intercept(CPUX86State *env, uint32_t type);
 #endif
 
 /* apic.c */
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 9d671297cf..2f7606bebf 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -412,80 +412,91 @@ void helper_clgi(CPUX86State *env)
     env->hflags2 &= ~HF2_GIF_MASK;
 }
 
-void cpu_svm_check_intercept_param(CPUX86State *env, uint32_t type,
-                                   uint64_t param, uintptr_t retaddr)
+bool cpu_svm_has_intercept(CPUX86State *env, uint32_t type)
 {
-    CPUState *cs = env_cpu(env);
-
-    if (likely(!(env->hflags & HF_GUEST_MASK))) {
-        return;
-    }
     switch (type) {
     case SVM_EXIT_READ_CR0 ... SVM_EXIT_READ_CR0 + 8:
         if (env->intercept_cr_read & (1 << (type - SVM_EXIT_READ_CR0))) {
-            cpu_vmexit(env, type, param, retaddr);
+            return true;
         }
         break;
     case SVM_EXIT_WRITE_CR0 ... SVM_EXIT_WRITE_CR0 + 8:
         if (env->intercept_cr_write & (1 << (type - SVM_EXIT_WRITE_CR0))) {
-            cpu_vmexit(env, type, param, retaddr);
+            return true;
         }
         break;
     case SVM_EXIT_READ_DR0 ... SVM_EXIT_READ_DR0 + 7:
         if (env->intercept_dr_read & (1 << (type - SVM_EXIT_READ_DR0))) {
-            cpu_vmexit(env, type, param, retaddr);
+            return true;
         }
         break;
     case SVM_EXIT_WRITE_DR0 ... SVM_EXIT_WRITE_DR0 + 7:
         if (env->intercept_dr_write & (1 << (type - SVM_EXIT_WRITE_DR0))) {
-            cpu_vmexit(env, type, param, retaddr);
+            return true;
         }
         break;
     case SVM_EXIT_EXCP_BASE ... SVM_EXIT_EXCP_BASE + 31:
         if (env->intercept_exceptions & (1 << (type - SVM_EXIT_EXCP_BASE))) {
-            cpu_vmexit(env, type, param, retaddr);
-        }
-        break;
-    case SVM_EXIT_MSR:
-        if (env->intercept & (1ULL << (SVM_EXIT_MSR - SVM_EXIT_INTR))) {
-            /* FIXME: this should be read in at vmrun (faster this way?) */
-            uint64_t addr = x86_ldq_phys(cs, env->vm_vmcb +
-                                     offsetof(struct vmcb,
-                                              control.msrpm_base_pa));
-            uint32_t t0, t1;
-
-            switch ((uint32_t)env->regs[R_ECX]) {
-            case 0 ... 0x1fff:
-                t0 = (env->regs[R_ECX] * 2) % 8;
-                t1 = (env->regs[R_ECX] * 2) / 8;
-                break;
-            case 0xc0000000 ... 0xc0001fff:
-                t0 = (8192 + env->regs[R_ECX] - 0xc0000000) * 2;
-                t1 = (t0 / 8);
-                t0 %= 8;
-                break;
-            case 0xc0010000 ... 0xc0011fff:
-                t0 = (16384 + env->regs[R_ECX] - 0xc0010000) * 2;
-                t1 = (t0 / 8);
-                t0 %= 8;
-                break;
-            default:
-                cpu_vmexit(env, type, param, retaddr);
-                t0 = 0;
-                t1 = 0;
-                break;
-            }
-            if (x86_ldub_phys(cs, addr + t1) & ((1 << param) << t0)) {
-                cpu_vmexit(env, type, param, retaddr);
-            }
+            return true;
         }
         break;
     default:
         if (env->intercept & (1ULL << (type - SVM_EXIT_INTR))) {
-            cpu_vmexit(env, type, param, retaddr);
+            return true;
         }
         break;
     }
+    return false;
+}
+
+void cpu_svm_check_intercept_param(CPUX86State *env, uint32_t type,
+                                   uint64_t param, uintptr_t retaddr)
+{
+    CPUState *cs = env_cpu(env);
+
+    if (likely(!(env->hflags & HF_GUEST_MASK))) {
+        return;
+    }
+
+    if (!cpu_svm_has_intercept(env, type)) {
+        return;
+    }
+
+    if (type == SVM_EXIT_MSR) {
+        /* FIXME: this should be read in at vmrun (faster this way?) */
+        uint64_t addr = x86_ldq_phys(cs, env->vm_vmcb +
+                                    offsetof(struct vmcb,
+                                            control.msrpm_base_pa));
+        uint32_t t0, t1;
+
+        switch ((uint32_t)env->regs[R_ECX]) {
+        case 0 ... 0x1fff:
+            t0 = (env->regs[R_ECX] * 2) % 8;
+            t1 = (env->regs[R_ECX] * 2) / 8;
+            break;
+        case 0xc0000000 ... 0xc0001fff:
+            t0 = (8192 + env->regs[R_ECX] - 0xc0000000) * 2;
+            t1 = (t0 / 8);
+            t0 %= 8;
+            break;
+        case 0xc0010000 ... 0xc0011fff:
+            t0 = (16384 + env->regs[R_ECX] - 0xc0010000) * 2;
+            t1 = (t0 / 8);
+            t0 %= 8;
+            break;
+        default:
+            cpu_vmexit(env, type, param, retaddr);
+            t0 = 0;
+            t1 = 0;
+            break;
+        }
+        if (x86_ldub_phys(cs, addr + t1) & ((1 << param) << t0)) {
+            cpu_vmexit(env, type, param, retaddr);
+        }
+        return;
+    }
+
+    cpu_vmexit(env, type, param, retaddr);
 }
 
 void helper_svm_check_intercept(CPUX86State *env, uint32_t type)
-- 
2.25.1

[PATCH v2 2/4] target/i386: Added consistency checks for VMRUN intercept and ASID

[Lara Lazier]

Zero VMRUN intercept and ASID should cause an immediate VMEXIT
during the consistency checks performed by VMRUN.
(AMD64 Architecture Programmer's Manual, V2, 15.5)

Signed-off-by: Lara Lazier <laramglazier@gmail.com>
---
 target/i386/tcg/sysemu/svm_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 2f7606bebf..902bf03fc3 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -72,6 +72,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     uint64_t nested_ctl;
     uint32_t event_inj;
     uint32_t int_ctl;
+    uint32_t asid;
 
     cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
 
@@ -154,9 +155,18 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
 
     nested_ctl = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
                                                           control.nested_ctl));
+    asid = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
+                                                          control.asid));
 
     env->nested_pg_mode = 0;
 
+    if (!cpu_svm_has_intercept(env, SVM_EXIT_VMRUN)) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+    if (asid == 0) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+
     if (nested_ctl & SVM_NPT_ENABLED) {
         env->nested_cr3 = x86_ldq_phys(cs,
                                 env->vm_vmcb + offsetof(struct vmcb,
-- 
2.25.1

[PATCH v2 3/4] target/i386: Added consistency checks for CR0

[Lara Lazier]

The combination of unset CD and set NW bit in CR0 is illegal.
CR0[63:32] are also reserved and need to be zero.
(AMD64 Architecture Programmer's Manual, V2, 15.5)

Signed-off-by: Lara Lazier <laramglazier@gmail.com>
---
 target/i386/cpu.h                   |  2 ++
 target/i386/svm.h                   |  2 ++
 target/i386/tcg/sysemu/svm_helper.c | 12 +++++++++---
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 59b9231ee2..6b9d04b33e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -224,6 +224,8 @@ typedef enum X86Seg {
 #define CR0_NE_MASK  (1U << 5)
 #define CR0_WP_MASK  (1U << 16)
 #define CR0_AM_MASK  (1U << 18)
+#define CR0_NW_MASK  (1U << 29)
+#define CR0_CD_MASK  (1U << 30)
 #define CR0_PG_MASK  (1U << 31)
 
 #define CR4_VME_MASK  (1U << 0)
diff --git a/target/i386/svm.h b/target/i386/svm.h
index 87965e5bc2..5098733053 100644
--- a/target/i386/svm.h
+++ b/target/i386/svm.h
@@ -135,6 +135,8 @@
 #define SVM_NPTEXIT_GPA     (1ULL << 32)
 #define SVM_NPTEXIT_GPT     (1ULL << 33)
 
+#define SVM_CR0_RESERVED_MASK 0xffffffff00000000U
+
 struct QEMU_PACKED vmcb_control_area {
 	uint16_t intercept_cr_read;
 	uint16_t intercept_cr_write;
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 902bf03fc3..1c2dbc1862 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -73,6 +73,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     uint32_t event_inj;
     uint32_t int_ctl;
     uint32_t asid;
+    uint64_t new_cr0;
 
     cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
 
@@ -192,13 +193,18 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     env->idt.limit = x86_ldl_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
                                                       save.idtr.limit));
 
+    new_cr0 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr0));
+    if (new_cr0 & SVM_CR0_RESERVED_MASK) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+    if ((new_cr0 & CR0_NW_MASK) && !(new_cr0 & CR0_CD_MASK)) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
     /* clear exit_info_2 so we behave like the real hardware */
     x86_stq_phys(cs,
              env->vm_vmcb + offsetof(struct vmcb, control.exit_info_2), 0);
 
-    cpu_x86_update_cr0(env, x86_ldq_phys(cs,
-                                     env->vm_vmcb + offsetof(struct vmcb,
-                                                             save.cr0)));
+    cpu_x86_update_cr0(env, new_cr0);
     cpu_x86_update_cr4(env, x86_ldq_phys(cs,
                                      env->vm_vmcb + offsetof(struct vmcb,
                                                              save.cr4)));
-- 
2.25.1

[PATCH v2 4/4] target/i386: Added Intercept CR0 writes check

[Lara Lazier]

When the selective CR0 write intercept is set, all writes to bits in
CR0 other than CR0.TS or CR0.MP cause a VMEXIT.

Signed-off-by: Lara Lazier <laramglazier@gmail.com>
---
 target/i386/tcg/sysemu/misc_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index 0cef2f1a4c..db0d8a9d79 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -84,6 +84,15 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
 {
     switch (reg) {
     case 0:
+        /*
+        * If we reach this point, the CR0 write intercept is disabled.
+        * But we could still exit if the hypervisor has requested the selective
+        * intercept for bits other than TS and MP
+        */
+        if (cpu_svm_has_intercept(env, SVM_EXIT_CR0_SEL_WRITE) &&
+            ((env->cr[0] ^ t0) & ~(CR0_TS_MASK | CR0_MP_MASK))) {
+            cpu_vmexit(env, SVM_EXIT_CR0_SEL_WRITE, 0, GETPC());
+        }
         cpu_x86_update_cr0(env, t0);
         break;
     case 3:
-- 
2.25.1

Re: [PATCH v2 1/4] target/i386: Refactored intercept checks into cpu_svm_has_intercept

[Paolo Bonzini]

On 16/06/21 14:39, Lara Lazier wrote:
>   cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
>                                 uint64_t param, uintptr_t retaddr)
>   { /* no-op */ }
> +bool cpu_svm_has_intercept(CPUX86State *env, uint32_t type)
> +{ return false; }

This needs to be declared "static inline", since it's in a header.

Paolo

Re: [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm

[Paolo Bonzini]

On 16/06/21 14:39, Lara Lazier wrote:
> Following the APM2 I added some checks to
> resolve the following tests in kvm-unit-tests for svm:
> 
>    * vmrun_intercept_check
>    * asid_zero
>    * sel_cr0_bug
>    * CR0 CD=0,NW=1: a0010011
>    * CR0 63:32: 180010011
>    * CR0 63:32: 1080010011
>    * CR0 63:32: 10080010011
>    * CR0 63:32: 100080010011
>    * CR0 63:32: 1000080010011
>    * CR0 63:32: 10000080010011
>    * CR0 63:32: 100000080010011
>    * CR0 63:32: 1000000080010011
> 
> v1->v2: introduced cpu_svm_has_intercept to avoid defining bitmasks for
>          intercepts
> 
> Lara Lazier (4):
>    target/i386: Refactored intercept checks into cpu_svm_has_intercept
>    target/i386: Added consistency checks for VMRUN intercept and ASID
>    target/i386: Added consistency checks for CR0
>    target/i386: Added Intercept CR0 writes check
> 
>   target/i386/cpu.h                    |   5 ++
>   target/i386/svm.h                    |   2 +
>   target/i386/tcg/sysemu/misc_helper.c |   9 ++
>   target/i386/tcg/sysemu/svm_helper.c  | 127 ++++++++++++++++-----------
>   4 files changed, 93 insertions(+), 50 deletions(-)
> 

Looks good.  I fixed the small issue in patch 1 and queued it, thanks. 
It will appear in a pull request later this week.

Paolo