* [PATCH v2 1/4] target/i386: Refactored intercept checks into cpu_svm_has_intercept
2021-06-16 12:39 [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Lara Lazier
@ 2021-06-16 12:39 ` Lara Lazier
2021-06-16 12:59 ` Paolo Bonzini
2021-06-16 12:39 ` [PATCH v2 2/4] target/i386: Added consistency checks for VMRUN intercept and ASID Lara Lazier
` (3 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Lara Lazier @ 2021-06-16 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/4] target/i386: Added consistency checks for VMRUN intercept and ASID
2021-06-16 12:39 [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Lara Lazier
2021-06-16 12:39 ` [PATCH v2 1/4] target/i386: Refactored intercept checks into cpu_svm_has_intercept Lara Lazier
@ 2021-06-16 12:39 ` Lara Lazier
2021-06-16 12:39 ` [PATCH v2 3/4] target/i386: Added consistency checks for CR0 Lara Lazier
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Lara Lazier @ 2021-06-16 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 3/4] target/i386: Added consistency checks for CR0
2021-06-16 12:39 [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Lara Lazier
2021-06-16 12:39 ` [PATCH v2 1/4] target/i386: Refactored intercept checks into cpu_svm_has_intercept Lara Lazier
2021-06-16 12:39 ` [PATCH v2 2/4] target/i386: Added consistency checks for VMRUN intercept and ASID Lara Lazier
@ 2021-06-16 12:39 ` Lara Lazier
2021-06-16 12:39 ` [PATCH v2 4/4] target/i386: Added Intercept CR0 writes check Lara Lazier
2021-06-16 13:00 ` [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Paolo Bonzini
4 siblings, 0 replies; 7+ messages in thread
From: Lara Lazier @ 2021-06-16 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 4/4] target/i386: Added Intercept CR0 writes check
2021-06-16 12:39 [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Lara Lazier
` (2 preceding siblings ...)
2021-06-16 12:39 ` [PATCH v2 3/4] target/i386: Added consistency checks for CR0 Lara Lazier
@ 2021-06-16 12:39 ` Lara Lazier
2021-06-16 13:00 ` [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Paolo Bonzini
4 siblings, 0 replies; 7+ messages in thread
From: Lara Lazier @ 2021-06-16 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm
2021-06-16 12:39 [PATCH v2 0/4] target/i386: Start fixing kvm-unit-tests for svm Lara Lazier
` (3 preceding siblings ...)
2021-06-16 12:39 ` [PATCH v2 4/4] target/i386: Added Intercept CR0 writes check Lara Lazier
@ 2021-06-16 13:00 ` Paolo Bonzini
4 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2021-06-16 13:00 UTC (permalink / raw)
To: Lara Lazier, qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread