[PATCH 0/3] Start fixing kvm-unit-tests for svm

[PATCH 0/3] 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

Lara Lazier (3):
  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                    |  4 ++++
 target/i386/svm.h                    |  3 +++
 target/i386/tcg/sysemu/misc_helper.c |  9 +++++++++
 target/i386/tcg/sysemu/svm_helper.c  | 22 +++++++++++++++++++---
 4 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/3] 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/svm.h                   |  2 ++
 target/i386/tcg/sysemu/svm_helper.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/target/i386/svm.h b/target/i386/svm.h
index 87965e5bc2..1c55d4f829 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_VMRUN_INTERCEPT (1ULL << 32)
+
 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 9d671297cf..ff826fe11a 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 (!(env->intercept & SVM_VMRUN_INTERCEPT)) {
+        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 2/3] 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                   |  1 +
 target/i386/tcg/sysemu/svm_helper.c | 12 +++++++++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ac3abea97c..46542513cc 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 1c55d4f829..7a88906907 100644
--- a/target/i386/svm.h
+++ b/target/i386/svm.h
@@ -136,6 +136,7 @@
 #define SVM_NPTEXIT_GPT     (1ULL << 33)
 
 #define SVM_VMRUN_INTERCEPT (1ULL << 32)
+#define SVM_CR0_RESERVED_MASK 0xffffffff00000000U
 
 struct QEMU_PACKED vmcb_control_area {
 	uint16_t intercept_cr_read;
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index ff826fe11a..63aaa53490 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 3/3] 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/cpu.h                    | 2 ++
 target/i386/tcg/sysemu/misc_helper.c | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 46542513cc..ff0ff97ca9 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -228,6 +228,8 @@ typedef enum X86Seg {
 #define CR0_CD_MASK  (1U << 30)
 #define CR0_PG_MASK  (1U << 31)
 
+#define INTERCEPT_SELECTIVE_CR0 (1ULL << 5)
+
 #define CR4_VME_MASK  (1U << 0)
 #define CR4_PVI_MASK  (1U << 1)
 #define CR4_TSD_MASK  (1U << 2)
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index 0cef2f1a4c..53117f47de 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 ((env->intercept & INTERCEPT_SELECTIVE_CR0) &&
+            ((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 1/3] target/i386: Added consistency checks for VMRUN intercept and ASID

[Paolo Bonzini]

On 14/06/21 12:09, Lara Lazier wrote:
> +#define SVM_VMRUN_INTERCEPT (1ULL << 32)
> +
>   struct QEMU_PACKED vmcb_control_area {
>   	uint16_t intercept_cr_read;
>   	uint16_t intercept_cr_write;

...

> +    if (!(env->intercept & SVM_VMRUN_INTERCEPT)) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }

Hi Lara,

as discussed in our weekly meeting, the only issue with these patch is a 
matter of aesthetics and maintainability more than functionality; 
namely, the duplication between SVM_VMRUN_INTERCEPT and SVM_EXIT_VMRUN, 
and likewise in patch 3 between INTERCEPT_SELECTIVE_CR0 and 
SVM_EXIT_CR0_SEL_WRITE.  Showing them side by side also makes it 
apparent that the names are not consistent, but it's even better to 
avoid the duplication altogether if possible.

In particular, one way to do so is to extract the intercept checks to a 
function that you can call like

     cpu_svm_has_intercept(env, SVM_EXIT_VMRUN)

so that the function computes the right bit of the bitmap based on the 
second argument.  Most of the code to do this is already in 
svm_helper.c's cpu_svm_check_intercept_param, which you're already 
familiar with.  cpu_svm_check_intercept_param can also be modified to 
call the new cpu_svm_has_intercept.

When your second version of the patches are ready, you can add the "-v2" 
argument to git format-patch and it will automatically start the 
subjects with "[PATCH v2 ...]" instead of just "[PATCH ...]".

Paolo