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

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

[Lara Lazier]

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

 * EFER.SVME: 1500
 * EFER.SVME: 500
 * Test EFER 9:8: 1700
 * Test EFER 63:16: 11500
 * Test EFER 63:16: 101500
 * Test EFER 63:16: 1001500
 * Test EFER 63:16: 10001500
 * Test EFER 63:16: 100001500
 * Test EFER 63:16: 1000001500
 * Test EFER 63:16: 10000001500
 * Test EFER 63:16: 100000001500
 * Test EFER 63:16: 1000000001500
 * Test EFER 63:16: 10000000001500
 * Test EFER 63:16: 100000000001500
 * Test EFER 63:16: 1000000000001500
 * EFER.LME=1, CR0.PG=1 and CR4.PAE=0
 * EFER.LME=1, CR0.PG=1 and CR0.PE=0
 * EFER.LME=1, CR0.PG=1, CR4.PAE=1, CS.L=1 and CS.D=1
 * Test CR3 63:0: 10000001007000
 * Test CR3 63:0: 20000001007000
 * Test CR3 63:0: 40000001007000
 * Test CR3 63:0: 80000001007000
 * Test CR3 63:0: 100000001007000
 * Test CR3 63:0: 200000001007000
 * Test CR3 63:0: 400000001007000
 * Test CR3 63:0: 800000001007000
 * Test CR3 63:0: 1000000001007000
 * Test CR3 63:0: 2000000001007000
 * Test CR3 63:0: 4000000001007000
 * Test CR3 63:0: 8000000001007000
 * Test CR3 63:0: 1007000
 * Test CR4 31:12: 1020
 * Test CR4 31:12: 2020
 * Test CR4 31:12: 4020
 * Test CR4 31:12: 8020
 * Test CR4 31:12: 80020
 * Test CR4 31:12: 800020
 * Test CR4 31:12: 1000020
 * Test CR4 31:12: 2000020
 * Test CR4 31:12: 4000020
 * Test CR4 31:12: 8000020
 * Test CR4 31:12: 10000020
 * Test CR4 31:12: 20000020
 * Test CR4 31:12: 40000020
 * Test CR4 31:12: 80000020
 * Test CR4 31:12: 1020
 * Test CR4 31:12: 2020
 * Test CR4 31:12: 4020
 * Test CR4 31:12: 8020
 * Test CR4 31:12: 80020
 * Test CR4 31:12: 800020
 * Test CR4 31:12: 1000020
 * Test CR4 31:12: 2000020
 * Test CR4 31:12: 4000020
 * Test CR4 31:12: 8000020
 * Test CR4 31:12: 10000020
 * Test CR4 31:12: 20000020
 * Test CR4 31:12: 40000020
 * Test CR4 31:12: 80000020
 * Test CR4 63:32: 100000020
 * Test CR4 63:32: 1000000020
 * Test CR4 63:32: 10000000020
 * Test CR4 63:32: 100000000020
 * Test CR4 63:32: 1000000000020
 * Test CR4 63:32: 10000000000020
 * Test CR4 63:32: 100000000000020
 * Test CR4 63:32: 1000000000000020
 * Test DR6 63:32: 1ffff0ff0
 * Test DR6 63:32: 10ffff0ff0
 * Test DR6 63:32: 100ffff0ff0
 * Test DR6 63:32: 1000ffff0ff0
 * Test DR6 63:32: 10000ffff0ff0
 * Test DR6 63:32: 100000ffff0ff0
 * Test DR6 63:32: 1000000ffff0ff0
 * Test DR6 63:32: 10000000ffff0ff0
 * Test DR7 63:32: 100000400
 * Test DR7 63:32: 1000000400
 * Test DR7 63:32: 10000000400
 * Test DR7 63:32: 100000000400
 * Test DR7 63:32: 1000000000400
 * Test DR7 63:32: 10000000000400
 * Test DR7 63:32: 100000000000400
 * Test DR7 63:32: 1000000000000400
 * Test MSRPM address: ffffffe000
 * Test MSRPM address: ffffffe001
 * Test MSRPM address: fffffff000
 * Test MSRPM address: 435000
 * Test MSRPM address: 435fff
 * Test IOPM address: ffffffc000
 * Test IOPM address: ffffffd000
 * Test IOPM address: ffffffdffe
 * Test IOPM address: ffffffe000
 * Test IOPM address: fffffff000
 * Test IOPM address: 438000
 * Test IOPM address: 438fff

Lara Lazier (4):
  target/i386: Added MSRPM and IOPM size check
  target/i386: Added DR6 and DR7 consistency checks
  target/i386: Added consistency checks for EFER
  target/i386: Added VMRUN consistency checks for CR3 and CR4

 target/i386/cpu.h                    | 31 +++++++++++
 target/i386/svm.h                    |  5 ++
 target/i386/tcg/sysemu/misc_helper.c |  6 +++
 target/i386/tcg/sysemu/svm_helper.c  | 80 +++++++++++++++++++++++++---
 4 files changed, 115 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/4] target/i386: Added MSRPM and IOPM size check

[Lara Lazier]

The address of the last entry in the MSRPM and
in the IOPM must be smaller than the largest physical address.
(APM2 15.10-15.11)

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

diff --git a/target/i386/svm.h b/target/i386/svm.h
index 5098733053..adc058dc76 100644
--- a/target/i386/svm.h
+++ b/target/i386/svm.h
@@ -137,6 +137,9 @@
 
 #define SVM_CR0_RESERVED_MASK 0xffffffff00000000U
 
+#define SVM_MSRPM_SIZE		(1ULL << 13)
+#define SVM_IOPM_SIZE		((1ULL << 13) + 1)
+
 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 1c2dbc1862..fa701829e5 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -68,6 +68,7 @@ static inline void svm_load_seg_cache(CPUX86State *env, hwaddr addr,
 void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
 {
     CPUState *cs = env_cpu(env);
+    X86CPU *cpu = env_archcpu(env);
     target_ulong addr;
     uint64_t nested_ctl;
     uint32_t event_inj;
@@ -159,6 +160,20 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     asid = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
                                                           control.asid));
 
+    uint64_t msrpm_base_pa = x86_ldq_phys(cs, env->vm_vmcb +
+                                    offsetof(struct vmcb,
+                                            control.msrpm_base_pa));
+    uint64_t iopm_base_pa = x86_ldq_phys(cs, env->vm_vmcb +
+                                 offsetof(struct vmcb, control.iopm_base_pa));
+
+    if ((msrpm_base_pa & ~0xfff) >= (1ull << cpu->phys_bits) - SVM_MSRPM_SIZE) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+
+    if ((iopm_base_pa & ~0xfff) >= (1ull << cpu->phys_bits) - SVM_IOPM_SIZE) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+
     env->nested_pg_mode = 0;
 
     if (!cpu_svm_has_intercept(env, SVM_EXIT_VMRUN)) {
-- 
2.25.1

[PATCH 2/4] target/i386: Added DR6 and DR7 consistency checks

[Lara Lazier]

DR6[63:32] and DR7[63:32] are reserved and need to be zero.
(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 | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/i386/svm.h b/target/i386/svm.h
index adc058dc76..e54670ef12 100644
--- a/target/i386/svm.h
+++ b/target/i386/svm.h
@@ -140,6 +140,8 @@
 #define SVM_MSRPM_SIZE		(1ULL << 13)
 #define SVM_IOPM_SIZE		((1ULL << 13) + 1)
 
+#define SVM_DR_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 fa701829e5..276c240f70 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -269,7 +269,13 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     env->dr[6] = x86_ldq_phys(cs,
                           env->vm_vmcb + offsetof(struct vmcb, save.dr6));
 
-    /* FIXME: guest state consistency checks */
+    if (env->dr[6] & SVM_DR_RESERVED_MASK) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+
+    if (env->dr[7] & SVM_DR_RESERVED_MASK) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
 
     switch (x86_ldub_phys(cs,
                       env->vm_vmcb + offsetof(struct vmcb, control.tlb_ctl))) {
-- 
2.25.1

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

[Lara Lazier]

EFER.SVME has to be set, and EFER[63:16], EFER[9], EFER[7:5]
are reserved and must be zero.
In Addition the combinations
 * EFER.LMA or EFER.LME is non-zero and the processor does not support LM
 * non-zero EFER.LME and CR0.PG and zero CR4.PAE
 * non-zero EFER.LME and CR0.PG and zero CR0.PE
 * non-zero EFER.LME, CR0.PG, CR4.PAE, CS.L and CS.D
are all invalid.
(AMD64 Architecture Programmer's Manual, V2, 15.5)

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

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f7fa5870b1..f5280b2951 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -464,6 +464,8 @@ typedef enum X86Seg {
 #define MSR_EFER_SVME  (1 << 12)
 #define MSR_EFER_FFXSR (1 << 14)
 
+#define MSR_EFER_RESERVED 0xffffffffffff02e0
+
 #define MSR_STAR                        0xc0000081
 #define MSR_LSTAR                       0xc0000082
 #define MSR_CSTAR                       0xc0000083
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 276c240f70..d652d6f9da 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -65,6 +65,41 @@ static inline void svm_load_seg_cache(CPUX86State *env, hwaddr addr,
                            sc->base, sc->limit, sc->flags);
 }
 
+static inline bool is_efer_invalid_state (CPUX86State *env)
+{
+    if (!(env->efer & MSR_EFER_SVME)) {
+        return true;
+    }
+
+    if (env->efer & MSR_EFER_RESERVED) {
+        return true;
+    }
+
+    if ((env->efer & (MSR_EFER_LMA | MSR_EFER_LME)) &&
+            !(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) {
+        return true;
+    }
+
+    if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK)
+                                && !(env->cr[4] & CR4_PAE_MASK)) {
+        return true;
+    }
+
+    if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK)
+                                && !(env->cr[0] & CR0_PE_MASK)) {
+        return true;
+    }
+
+    if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK)
+                                && (env->cr[4] & CR4_PAE_MASK)
+                                && (env->segs[R_CS].flags & DESC_L_MASK)
+                                && (env->segs[R_CS].flags & DESC_B_MASK)) {
+        return true;
+    }
+
+    return false;
+}
+
 void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
 {
     CPUState *cs = env_cpu(env);
@@ -277,6 +312,10 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
         cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
     }
 
+    if (is_efer_invalid_state(env)) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+
     switch (x86_ldub_phys(cs,
                       env->vm_vmcb + offsetof(struct vmcb, control.tlb_ctl))) {
     case TLB_CONTROL_DO_NOTHING:
-- 
2.25.1

[PATCH 4/4] target/i386: Added VMRUN consistency checks for CR3 and CR4

[Lara Lazier]

All MBZ bits in CR3 and CR4 must be zero. (APM2 15.5)
Added reserved bitmask for CR4 and added checks in both
helper_vmrun and helper_write_crN.

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

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f5280b2951..0368551a66 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -240,6 +240,7 @@ typedef enum X86Seg {
 #define CR4_OSFXSR_SHIFT 9
 #define CR4_OSFXSR_MASK (1U << CR4_OSFXSR_SHIFT)
 #define CR4_OSXMMEXCPT_MASK  (1U << 10)
+#define CR4_UMIP_MASK   (1U << 11)
 #define CR4_LA57_MASK   (1U << 12)
 #define CR4_VMXE_MASK   (1U << 13)
 #define CR4_SMXE_MASK   (1U << 14)
@@ -251,6 +252,34 @@ typedef enum X86Seg {
 #define CR4_PKE_MASK   (1U << 22)
 #define CR4_PKS_MASK   (1U << 24)
 
+#define CR4_RESERVED_MASK \
+(~(unsigned long)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
+                | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
+                | CR4_MCE_MASK | CR4_PGE_MASK | CR4_PCE_MASK \
+                | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK |CR4_UMIP_MASK \
+                | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
+                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK))
+
+#define cr4_reserved_bits(env) \
+({ \
+    uint64_t __reserved_bits = CR4_RESERVED_MASK; \
+    if (!env->features[FEAT_XSAVE]) \
+        __reserved_bits |= CR4_OSXSAVE_MASK; \
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SMEP)) \
+        __reserved_bits |= CR4_SMEP_MASK; \
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SMAP)) \
+        __reserved_bits |= CR4_SMAP_MASK; \
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FSGSBASE)) \
+        __reserved_bits |= CR4_FSGSBASE_MASK; \
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKU)) \
+        __reserved_bits |= CR4_PKE_MASK; \
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_LA57)) \
+        __reserved_bits |= CR4_LA57_MASK; \
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_UMIP)) \
+        __reserved_bits |= CR4_UMIP_MASK; \
+    __reserved_bits; \
+})
+
 #define DR6_BD          (1 << 13)
 #define DR6_BS          (1 << 14)
 #define DR6_BT          (1 << 15)
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index db0d8a9d79..cdb4826987 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -96,9 +96,15 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
         cpu_x86_update_cr0(env, t0);
         break;
     case 3:
+        if (t0 & ((~0UL) << env_archcpu(env)->phys_bits)) {
+            cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+        }
         cpu_x86_update_cr3(env, t0);
         break;
     case 4:
+        if (t0 & cr4_reserved_bits(env)) {
+            cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+        }
         if (((t0 ^ env->cr[4]) & CR4_LA57_MASK) &&
             (env->hflags & HF_CS64_MASK)) {
             raise_exception_ra(env, EXCP0D_GPF, GETPC());
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index d652d6f9da..e3a4fa74d2 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -110,6 +110,8 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     uint32_t int_ctl;
     uint32_t asid;
     uint64_t new_cr0;
+    uint64_t new_cr3;
+    uint64_t new_cr4;
 
     cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
 
@@ -250,17 +252,21 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     if ((new_cr0 & CR0_NW_MASK) && !(new_cr0 & CR0_CD_MASK)) {
         cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
     }
+    new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3));
+    if (new_cr3 & ((~0UL) << cpu->phys_bits)) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
+    new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4));
+    if (new_cr4 & cr4_reserved_bits(env)) {
+        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, new_cr0);
-    cpu_x86_update_cr4(env, x86_ldq_phys(cs,
-                                     env->vm_vmcb + offsetof(struct vmcb,
-                                                             save.cr4)));
-    cpu_x86_update_cr3(env, x86_ldq_phys(cs,
-                                     env->vm_vmcb + offsetof(struct vmcb,
-                                                             save.cr3)));
+    cpu_x86_update_cr4(env, new_cr4);
+    cpu_x86_update_cr3(env, new_cr3);
     env->cr[2] = x86_ldq_phys(cs,
                           env->vm_vmcb + offsetof(struct vmcb, save.cr2));
     int_ctl = x86_ldl_phys(cs,
-- 
2.25.1

Re: [PATCH 1/4] target/i386: Added MSRPM and IOPM size check

[Paolo Bonzini]

On 05/07/21 10:17, Lara Lazier wrote:
> The address of the last entry in the MSRPM and
> in the IOPM must be smaller than the largest physical address.
> (APM2 15.10-15.11)
> 
> Signed-off-by: Lara Lazier <laramglazier@gmail.com>
> ---
>   target/i386/svm.h                   |  3 +++
>   target/i386/tcg/sysemu/svm_helper.c | 15 +++++++++++++++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/target/i386/svm.h b/target/i386/svm.h
> index 5098733053..adc058dc76 100644
> --- a/target/i386/svm.h
> +++ b/target/i386/svm.h
> @@ -137,6 +137,9 @@
>   
>   #define SVM_CR0_RESERVED_MASK 0xffffffff00000000U
>   
> +#define SVM_MSRPM_SIZE		(1ULL << 13)
> +#define SVM_IOPM_SIZE		((1ULL << 13) + 1)
> +
>   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 1c2dbc1862..fa701829e5 100644
> --- a/target/i386/tcg/sysemu/svm_helper.c
> +++ b/target/i386/tcg/sysemu/svm_helper.c
> @@ -68,6 +68,7 @@ static inline void svm_load_seg_cache(CPUX86State *env, hwaddr addr,
>   void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
>   {
>       CPUState *cs = env_cpu(env);
> +    X86CPU *cpu = env_archcpu(env);
>       target_ulong addr;
>       uint64_t nested_ctl;
>       uint32_t event_inj;
> @@ -159,6 +160,20 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
>       asid = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
>                                                             control.asid));
>   
> +    uint64_t msrpm_base_pa = x86_ldq_phys(cs, env->vm_vmcb +
> +                                    offsetof(struct vmcb,
> +                                            control.msrpm_base_pa));
> +    uint64_t iopm_base_pa = x86_ldq_phys(cs, env->vm_vmcb +
> +                                 offsetof(struct vmcb, control.iopm_base_pa));
> +
> +    if ((msrpm_base_pa & ~0xfff) >= (1ull << cpu->phys_bits) - SVM_MSRPM_SIZE) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }
> +
> +    if ((iopm_base_pa & ~0xfff) >= (1ull << cpu->phys_bits) - SVM_IOPM_SIZE) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }
> +
>       env->nested_pg_mode = 0;
>   
>       if (!cpu_svm_has_intercept(env, SVM_EXIT_VMRUN)) {
> 

Queued, thanks.

Paolo

Re: [PATCH 2/4] target/i386: Added DR6 and DR7 consistency checks

[Paolo Bonzini]

On 05/07/21 10:18, Lara Lazier wrote:
> DR6[63:32] and DR7[63:32] are reserved and need to be zero.
> (AMD64 Architecture Programmer's Manual, V2, 15.5)
> 
> Signed-off-by: Lara Lazier <laramglazier@gmail.com>

The checks need to be disabled on 32-bit builds where env->dr[6] and 
env->dr[7] is a 32-bit value (compilers or static analyzers would 
complain about always-zero conditions) but otherwise the patch is fine. 
  When you fetch from the QEMU repository and rebase, you will be able 
to use my changes just with "git rebase --skip".

Queued, thanks.

Paolo

> ---
>   target/i386/svm.h                   | 2 ++
>   target/i386/tcg/sysemu/svm_helper.c | 8 +++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/svm.h b/target/i386/svm.h
> index adc058dc76..e54670ef12 100644
> --- a/target/i386/svm.h
> +++ b/target/i386/svm.h
> @@ -140,6 +140,8 @@
>   #define SVM_MSRPM_SIZE		(1ULL << 13)
>   #define SVM_IOPM_SIZE		((1ULL << 13) + 1)
>   
> +#define SVM_DR_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 fa701829e5..276c240f70 100644
> --- a/target/i386/tcg/sysemu/svm_helper.c
> +++ b/target/i386/tcg/sysemu/svm_helper.c
> @@ -269,7 +269,13 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
>       env->dr[6] = x86_ldq_phys(cs,
>                             env->vm_vmcb + offsetof(struct vmcb, save.dr6));
>   
> -    /* FIXME: guest state consistency checks */
> +    if (env->dr[6] & SVM_DR_RESERVED_MASK) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }
> +
> +    if (env->dr[7] & SVM_DR_RESERVED_MASK) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }
>   
>       switch (x86_ldub_phys(cs,
>                         env->vm_vmcb + offsetof(struct vmcb, control.tlb_ctl))) {
> 

Re: [PATCH 3/4] target/i386: Added consistency checks for EFER

[Paolo Bonzini]

> EFER.SVME has to be set, and EFER[63:16], EFER[9], EFER[7:5]
> are reserved and must be zero.

My version of the manual says "any MBZ [must-be-zero] bit of EFER is 
set", so that would be 7:1 (not 7:5), 9 and 63:16.  In QEMU bits 13 and 
15 are also unimplemented and thus must-be-zero.

On 05/07/21 10:18, Lara Lazier wrote:
> +#define MSR_EFER_RESERVED 0xffffffffffff02e0
> +

This has the same issue with 32-bit compilation, except in this case the 
check *is* needed on 32-bit builds just without bits 63:32 set.

The obvious way here would be to add a #ifdef, but that's less 
maintainable than the slightly ugly:

#define MSR_EFER_RESERVED               ((target_ulong)(int)0xffff02e0u)

(where I was too lazy to compute the right mask for the bits I listed 
above...).

Paolo

Re: [PATCH 4/4] target/i386: Added VMRUN consistency checks for CR3 and CR4

[Paolo Bonzini]

On 05/07/21 10:18, Lara Lazier wrote:
> +#define CR4_RESERVED_MASK \
> +(~(unsigned long)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
> +                | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
> +                | CR4_MCE_MASK | CR4_PGE_MASK | CR4_PCE_MASK \
> +                | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK |CR4_UMIP_MASK \
> +                | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
> +                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK))
> +

This ~ trick could also be useful for EFER, very nice!

Just a couple changes required:

1) CR4_PKS_MASK is missing here and in cr4_reserved_bits (TCG supports 
it but KVM does not)

2) the cast should be to target_ulong (to cover the case of 32-bit host 
and 64-bit emulated processor)


In addition, as discussed on our weekly call CR3 checks are not complete 
so it's probably best to focus on CR4 for this patch and split CR3 to a 
different one.

Paolo