* [Qemu-devel] [PATCH 1/7] KVM: i386: Use symbolic constant for #DB/#BP exception constants
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
2019-06-15 0:46 ` Liran Alon
2019-06-15 0:42 ` [Qemu-devel] [PATCH 2/7] KVM: i386: Re-inject #DB to guest with updated DR6 Paolo Bonzini
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
From: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3b29ce5c0d..c8d8196e71 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2994,9 +2994,9 @@ static int kvm_guest_debug_workarounds(X86CPU *cpu)
unsigned long reinject_trap = 0;
if (!kvm_has_vcpu_events()) {
- if (env->exception_injected == 1) {
+ if (env->exception_injected == EXCP01_DB) {
reinject_trap = KVM_GUESTDBG_INJECT_DB;
- } else if (env->exception_injected == 3) {
+ } else if (env->exception_injected == EXCP03_INT3) {
reinject_trap = KVM_GUESTDBG_INJECT_BP;
}
env->exception_injected = -1;
@@ -3508,7 +3508,7 @@ static int kvm_handle_debug(X86CPU *cpu,
int ret = 0;
int n;
- if (arch_info->exception == 1) {
+ if (arch_info->exception == EXCP01_DB) {
if (arch_info->dr6 & (1 << 14)) {
if (cs->singlestep_enabled) {
ret = EXCP_DEBUG;
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] KVM: i386: Use symbolic constant for #DB/#BP exception constants
2019-06-15 0:42 ` [Qemu-devel] [PATCH 1/7] KVM: i386: Use symbolic constant for #DB/#BP exception constants Paolo Bonzini
@ 2019-06-15 0:46 ` Liran Alon
0 siblings, 0 replies; 19+ messages in thread
From: Liran Alon @ 2019-06-15 0:46 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, nikita.leshchenko
> On 15 Jun 2019, at 3:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Liran Alon <liran.alon@oracle.com>
>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/kvm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 3b29ce5c0d..c8d8196e71 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -2994,9 +2994,9 @@ static int kvm_guest_debug_workarounds(X86CPU *cpu)
> unsigned long reinject_trap = 0;
>
> if (!kvm_has_vcpu_events()) {
> - if (env->exception_injected == 1) {
> + if (env->exception_injected == EXCP01_DB) {
> reinject_trap = KVM_GUESTDBG_INJECT_DB;
> - } else if (env->exception_injected == 3) {
> + } else if (env->exception_injected == EXCP03_INT3) {
> reinject_trap = KVM_GUESTDBG_INJECT_BP;
> }
> env->exception_injected = -1;
> @@ -3508,7 +3508,7 @@ static int kvm_handle_debug(X86CPU *cpu,
> int ret = 0;
> int n;
>
> - if (arch_info->exception == 1) {
> + if (arch_info->exception == EXCP01_DB) {
> if (arch_info->dr6 & (1 << 14)) {
Note: In the patch-series I will submit, I also replaced this “1 << 14” with DR6_BS.
> if (cs->singlestep_enabled) {
> ret = EXCP_DEBUG;
> --
> 2.21.0
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 2/7] KVM: i386: Re-inject #DB to guest with updated DR6
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
2019-06-15 0:42 ` [Qemu-devel] [PATCH 1/7] KVM: i386: Use symbolic constant for #DB/#BP exception constants Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
2019-06-15 0:42 ` [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD Paolo Bonzini
` (4 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
From: Liran Alon <liran.alon@oracle.com>
If userspace (QEMU) debug guest, when #DB is raised in guest and
intercepted by KVM, KVM forwards information on #DB to userspace
instead of injecting #DB to guest.
While doing so, KVM don't update vCPU DR6 but instead report the #DB DR6
value to userspace for further handling.
See KVM's handle_exception() DB_VECTOR handler.
QEMU handler for this case is kvm_handle_debug(). This handler basically
checks if #DB is related to one of user set hardware breakpoints and if
not, it re-inject #DB into guest.
The re-injection is done by setting env->exception_injected to #DB which
will later be passed as events.exception.nr to KVM_SET_VCPU_EVENTS ioctl
by kvm_put_vcpu_events().
However, in case userspace re-injects #DB, KVM expects userspace to set
vCPU DR6 as reported to userspace when #DB was intercepted! Otherwise,
KVM_REQ_EVENT handler will inject #DB with wrong DR6 to guest.
Fix this issue by updating vCPU DR6 appropriately when re-inject #DB to
guest.
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index c8d8196e71..53f95b02a0 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -3546,6 +3546,9 @@ static int kvm_handle_debug(X86CPU *cpu,
/* pass to guest */
env->exception_injected = arch_info->exception;
env->has_error_code = 0;
+ if (arch_info->exception == EXCP01_DB) {
+ env->dr[6] = arch_info->dr6;
+ }
}
return ret;
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
2019-06-15 0:42 ` [Qemu-devel] [PATCH 1/7] KVM: i386: Use symbolic constant for #DB/#BP exception constants Paolo Bonzini
2019-06-15 0:42 ` [Qemu-devel] [PATCH 2/7] KVM: i386: Re-inject #DB to guest with updated DR6 Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
2019-06-15 0:57 ` Liran Alon
2019-06-15 0:42 ` [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs Paolo Bonzini
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
From: Liran Alon <liran.alon@oracle.com>
Kernel commit c4f55198c7c2 ("kvm: x86: Introduce KVM_CAP_EXCEPTION_PAYLOAD")
introduced a new KVM capability which allows userspace to correctly
distinguish between pending and injected exceptions.
This distinguish is important in case of nested virtualization scenarios
because a L2 pending exception can still be intercepted by the L1 hypervisor
while a L2 injected exception cannot.
Furthermore, when an exception is attempted to be injected by QEMU,
QEMU should specify the exception payload (CR2 in case of #PF or
DR6 in case of #DB) instead of having the payload already delivered in
the respective vCPU register. Because in case exception is injected to
L2 guest and is intercepted by L1 hypervisor, then payload needs to be
reported to L1 intercept (VMExit handler) while still preserving
respective vCPU register unchanged.
This commit adds support for QEMU to properly utilise this new KVM
capability (KVM_CAP_EXCEPTION_PAYLOAD).
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 10 ++---
target/i386/cpu.h | 13 +++++-
target/i386/hvf/hvf.c | 10 +++--
target/i386/hvf/x86hvf.c | 4 +-
target/i386/kvm.c | 95 +++++++++++++++++++++++++++++++++-------
target/i386/machine.c | 61 +++++++++++++++++++++++++-
6 files changed, 163 insertions(+), 30 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c1ab86d63e..4e19969111 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4777,7 +4777,9 @@ static void x86_cpu_reset(CPUState *s)
memset(env->mtrr_fixed, 0, sizeof(env->mtrr_fixed));
env->interrupt_injected = -1;
- env->exception_injected = -1;
+ env->exception_nr = -1;
+ env->exception_pending = 0;
+ env->exception_injected = 0;
env->nmi_injected = false;
#if !defined(CONFIG_USER_ONLY)
/* We hard-wire the BSP to the first CPU. */
@@ -5173,12 +5175,6 @@ static int x86_cpu_filter_features(X86CPU *cpu)
return rv;
}
-#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
- (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
- (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
-#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
- (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
- (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
{
CPUState *cs = CPU(dev);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index bd06523a53..bbeb7a9521 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -729,6 +729,13 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
#define CPUID_VENDOR_HYGON "HygonGenuine"
+#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
+ (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
+ (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
+#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
+ (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
+ (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+
#define CPUID_MWAIT_IBE (1U << 1) /* Interrupts can exit capability */
#define CPUID_MWAIT_EMX (1U << 0) /* enumeration supported */
@@ -1332,10 +1339,14 @@ typedef struct CPUX86State {
/* For KVM */
uint32_t mp_state;
- int32_t exception_injected;
+ int32_t exception_nr;
int32_t interrupt_injected;
uint8_t soft_interrupt;
+ uint8_t exception_pending;
+ uint8_t exception_injected;
uint8_t has_error_code;
+ uint8_t exception_has_payload;
+ uint64_t exception_payload;
uint32_t ins_len;
uint32_t sipi_vector;
bool tsc_valid;
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 2751c8125c..dc4bb63536 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -605,7 +605,9 @@ static void hvf_store_events(CPUState *cpu, uint32_t ins_len, uint64_t idtvec_in
X86CPU *x86_cpu = X86_CPU(cpu);
CPUX86State *env = &x86_cpu->env;
- env->exception_injected = -1;
+ env->exception_nr = -1;
+ env->exception_pending = 0;
+ env->exception_injected = 0;
env->interrupt_injected = -1;
env->nmi_injected = false;
if (idtvec_info & VMCS_IDT_VEC_VALID) {
@@ -619,7 +621,8 @@ static void hvf_store_events(CPUState *cpu, uint32_t ins_len, uint64_t idtvec_in
break;
case VMCS_IDT_VEC_HWEXCEPTION:
case VMCS_IDT_VEC_SWEXCEPTION:
- env->exception_injected = idtvec_info & VMCS_IDT_VEC_VECNUM;
+ env->exception_nr = idtvec_info & VMCS_IDT_VEC_VECNUM;
+ env->exception_injected = 1;
break;
case VMCS_IDT_VEC_PRIV_SWEXCEPTION:
default:
@@ -912,7 +915,8 @@ int hvf_vcpu_exec(CPUState *cpu)
macvm_set_rip(cpu, rip + ins_len);
break;
case VMX_REASON_VMCALL:
- env->exception_injected = EXCP0D_GPF;
+ env->exception_nr = EXCP0D_GPF;
+ env->exception_injected = 1;
env->has_error_code = true;
env->error_code = 0;
break;
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index df8e946fbc..e0ea02d631 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -362,8 +362,8 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
if (env->interrupt_injected != -1) {
vector = env->interrupt_injected;
intr_type = VMCS_INTR_T_SWINTR;
- } else if (env->exception_injected != -1) {
- vector = env->exception_injected;
+ } else if (env->exception_nr != -1) {
+ vector = env->exception_nr;
if (vector == EXCP03_INT3 || vector == EXCP04_INTO) {
intr_type = VMCS_INTR_T_SWEXCEPTION;
} else {
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 53f95b02a0..dca76830ec 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -104,6 +104,7 @@ static uint32_t num_architectural_pmu_fixed_counters;
static int has_xsave;
static int has_xcrs;
static int has_pit_state2;
+static int has_exception_payload;
static bool has_msr_mcg_ext_ctl;
@@ -584,15 +585,51 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
/* Hope we are lucky for AO MCE */
}
+static void kvm_reset_exception(CPUX86State *env)
+{
+ env->exception_nr = -1;
+ env->exception_pending = 0;
+ env->exception_injected = 0;
+}
+
+static void kvm_queue_exception(CPUX86State *env,
+ int32_t exception_nr,
+ uint8_t exception_has_payload,
+ uint64_t exception_payload)
+{
+ assert(env->exception_nr == -1);
+ assert(!env->exception_pending);
+ assert(!env->exception_injected);
+
+ env->exception_nr = exception_nr;
+
+ if (has_exception_payload) {
+ env->exception_pending = 1;
+
+ env->exception_has_payload = exception_has_payload;
+ env->exception_payload = exception_payload;
+ } else {
+ env->exception_injected = 1;
+
+ if (exception_has_payload) {
+ if (exception_nr == EXCP01_DB) {
+ env->dr[6] = exception_payload;
+ } else if (exception_nr == EXCP0E_PAGE) {
+ env->cr[2] = exception_payload;
+ }
+ }
+ }
+}
+
static int kvm_inject_mce_oldstyle(X86CPU *cpu)
{
CPUX86State *env = &cpu->env;
- if (!kvm_has_vcpu_events() && env->exception_injected == EXCP12_MCHK) {
+ if (!kvm_has_vcpu_events() && env->exception_nr == EXCP12_MCHK) {
unsigned int bank, bank_num = env->mcg_cap & 0xff;
struct kvm_x86_mce mce;
- env->exception_injected = -1;
+ kvm_reset_exception(env);
/*
* There must be at least one bank in use if an MCE is pending.
@@ -1573,6 +1610,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
hv_vpindex_settable = kvm_check_extension(s, KVM_CAP_HYPERV_VP_INDEX);
+ has_exception_payload = kvm_check_extension(s, KVM_CAP_EXCEPTION_PAYLOAD);
+ if (has_exception_payload) {
+ ret = kvm_vm_enable_cap(s, KVM_CAP_EXCEPTION_PAYLOAD, 0, true);
+ if (ret < 0) {
+ error_report("kvm: Failed to enable exception payload cap: %s",
+ strerror(-ret));
+ return ret;
+ }
+ }
+
ret = kvm_get_supported_msrs(s);
if (ret < 0) {
return ret;
@@ -2877,8 +2924,16 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
return 0;
}
- events.exception.injected = (env->exception_injected >= 0);
- events.exception.nr = env->exception_injected;
+ events.flags = 0;
+
+ if (has_exception_payload) {
+ events.flags |= KVM_VCPUEVENT_VALID_PAYLOAD;
+ events.exception.pending = env->exception_pending;
+ events.exception_has_payload = env->exception_has_payload;
+ events.exception_payload = env->exception_payload;
+ }
+ events.exception.nr = env->exception_nr;
+ events.exception.injected = env->exception_injected;
events.exception.has_error_code = env->has_error_code;
events.exception.error_code = env->error_code;
@@ -2891,7 +2946,6 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
events.nmi.masked = !!(env->hflags2 & HF2_NMI_MASK);
events.sipi_vector = env->sipi_vector;
- events.flags = 0;
if (has_msr_smbase) {
events.smi.smm = !!(env->hflags & HF_SMM_MASK);
@@ -2941,8 +2995,19 @@ static int kvm_get_vcpu_events(X86CPU *cpu)
if (ret < 0) {
return ret;
}
- env->exception_injected =
- events.exception.injected ? events.exception.nr : -1;
+
+ if (events.flags & KVM_VCPUEVENT_VALID_PAYLOAD) {
+ env->exception_pending = events.exception.pending;
+ env->exception_has_payload = events.exception_has_payload;
+ env->exception_payload = events.exception_payload;
+ } else {
+ env->exception_pending = 0;
+ env->exception_has_payload = false;
+ }
+ env->exception_injected = events.exception.injected;
+ env->exception_nr =
+ (env->exception_pending || env->exception_injected) ?
+ events.exception.nr : -1;
env->has_error_code = events.exception.has_error_code;
env->error_code = events.exception.error_code;
@@ -2994,12 +3059,12 @@ static int kvm_guest_debug_workarounds(X86CPU *cpu)
unsigned long reinject_trap = 0;
if (!kvm_has_vcpu_events()) {
- if (env->exception_injected == EXCP01_DB) {
+ if (env->exception_nr == EXCP01_DB) {
reinject_trap = KVM_GUESTDBG_INJECT_DB;
} else if (env->exception_injected == EXCP03_INT3) {
reinject_trap = KVM_GUESTDBG_INJECT_BP;
}
- env->exception_injected = -1;
+ kvm_reset_exception(env);
}
/*
@@ -3320,13 +3385,13 @@ int kvm_arch_process_async_events(CPUState *cs)
kvm_cpu_synchronize_state(cs);
- if (env->exception_injected == EXCP08_DBLE) {
+ if (env->exception_nr == EXCP08_DBLE) {
/* this means triple fault */
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
cs->exit_request = 1;
return 0;
}
- env->exception_injected = EXCP12_MCHK;
+ kvm_queue_exception(env, EXCP12_MCHK, 0, 0);
env->has_error_code = 0;
cs->halted = 0;
@@ -3541,14 +3606,12 @@ static int kvm_handle_debug(X86CPU *cpu,
}
if (ret == 0) {
cpu_synchronize_state(cs);
- assert(env->exception_injected == -1);
+ assert(env->exception_nr == -1);
/* pass to guest */
- env->exception_injected = arch_info->exception;
+ kvm_queue_exception(env, arch_info->exception,
+ EXCP01_DB, arch_info->dr6);
env->has_error_code = 0;
- if (arch_info->exception == EXCP01_DB) {
- env->dr[6] = arch_info->dr6;
- }
}
return ret;
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 225b5d433b..41460be54b 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -199,6 +199,21 @@ static const VMStateDescription vmstate_fpreg = {
}
};
+static bool is_vmx_enabled(CPUX86State *env)
+{
+ return (IS_INTEL_CPU(env) && (env->cr[4] & CR4_VMXE_MASK));
+}
+
+static bool is_svm_enabled(CPUX86State *env)
+{
+ return (IS_AMD_CPU(env) && (env->efer & MSR_EFER_SVME));
+}
+
+static bool is_nested_virt_enabled(CPUX86State *env)
+{
+ return (is_vmx_enabled(env) || is_svm_enabled(env));
+}
+
static int cpu_pre_save(void *opaque)
{
X86CPU *cpu = opaque;
@@ -278,6 +293,23 @@ static int cpu_post_load(void *opaque, int version_id)
env->hflags &= ~HF_CPL_MASK;
env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
+ /*
+ * There are cases that we can get valid exception_nr with both
+ * exception_pending and exception_clear being cleared. This can happen in
+ * one of the following scenarios:
+ * 1) Source is older QEMU without KVM_CAP_EXCEPTION_PAYLOAD support.
+ * 2) Source is running on kernel without KVM_CAP_EXCEPTION_PAYLOAD support.
+ * 3) "cpu/exception_info" subsection not sent because there is no exception
+ * pending or guest wasn't running L2.
+ *
+ * In those cases, we can just deduce that a valid exception_nr means
+ * we can treat the exception as already injected.
+ */
+ if ((env->exception_nr != -1) &&
+ !env->exception_pending && !env->exception_injected) {
+ env->exception_injected = 1;
+ }
+
env->fpstt = (env->fpus_vmstate >> 11) & 7;
env->fpus = env->fpus_vmstate & ~0x3800;
env->fptag_vmstate ^= 0xff;
@@ -323,6 +355,32 @@ static bool steal_time_msr_needed(void *opaque)
return cpu->env.steal_time_msr != 0;
}
+static bool exception_info_needed(void *opaque)
+{
+ X86CPU *cpu = opaque;
+
+ /*
+ * Differenting between pending and injected exceptions
+ * is only important when running L2.
+ */
+ return (cpu->env.exception_pending &&
+ is_nested_virt_enabled(&cpu->env));
+}
+
+static const VMStateDescription vmstate_exception_info = {
+ .name = "cpu/exception_info",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = exception_info_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(env.exception_pending, X86CPU),
+ VMSTATE_UINT8(env.exception_injected, X86CPU),
+ VMSTATE_UINT8(env.exception_has_payload, X86CPU),
+ VMSTATE_UINT64(env.exception_payload, X86CPU),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_steal_time_msr = {
.name = "cpu/steal_time_msr",
.version_id = 1,
@@ -1035,7 +1093,7 @@ VMStateDescription vmstate_x86_cpu = {
VMSTATE_INT32(env.interrupt_injected, X86CPU),
VMSTATE_UINT32(env.mp_state, X86CPU),
VMSTATE_UINT64(env.tsc, X86CPU),
- VMSTATE_INT32(env.exception_injected, X86CPU),
+ VMSTATE_INT32(env.exception_nr, X86CPU),
VMSTATE_UINT8(env.soft_interrupt, X86CPU),
VMSTATE_UINT8(env.nmi_injected, X86CPU),
VMSTATE_UINT8(env.nmi_pending, X86CPU),
@@ -1059,6 +1117,7 @@ VMStateDescription vmstate_x86_cpu = {
/* The above list is not sorted /wrt version numbers, watch out! */
},
.subsections = (const VMStateDescription*[]) {
+ &vmstate_exception_info,
&vmstate_async_pf_msr,
&vmstate_pv_eoi_msr,
&vmstate_steal_time_msr,
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD
2019-06-15 0:42 ` [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD Paolo Bonzini
@ 2019-06-15 0:57 ` Liran Alon
2019-06-16 12:38 ` Liran Alon
0 siblings, 1 reply; 19+ messages in thread
From: Liran Alon @ 2019-06-15 0:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, nikita.leshchenko
> On 15 Jun 2019, at 3:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Liran Alon <liran.alon@oracle.com>
>
> Kernel commit c4f55198c7c2 ("kvm: x86: Introduce KVM_CAP_EXCEPTION_PAYLOAD")
> introduced a new KVM capability which allows userspace to correctly
> distinguish between pending and injected exceptions.
>
> This distinguish is important in case of nested virtualization scenarios
> because a L2 pending exception can still be intercepted by the L1 hypervisor
> while a L2 injected exception cannot.
>
> Furthermore, when an exception is attempted to be injected by QEMU,
> QEMU should specify the exception payload (CR2 in case of #PF or
> DR6 in case of #DB) instead of having the payload already delivered in
> the respective vCPU register. Because in case exception is injected to
> L2 guest and is intercepted by L1 hypervisor, then payload needs to be
> reported to L1 intercept (VMExit handler) while still preserving
> respective vCPU register unchanged.
>
> This commit adds support for QEMU to properly utilise this new KVM
> capability (KVM_CAP_EXCEPTION_PAYLOAD).
>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/cpu.c | 10 ++---
> target/i386/cpu.h | 13 +++++-
> target/i386/hvf/hvf.c | 10 +++--
> target/i386/hvf/x86hvf.c | 4 +-
> target/i386/kvm.c | 95 +++++++++++++++++++++++++++++++++-------
> target/i386/machine.c | 61 +++++++++++++++++++++++++-
> 6 files changed, 163 insertions(+), 30 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c1ab86d63e..4e19969111 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4777,7 +4777,9 @@ static void x86_cpu_reset(CPUState *s)
> memset(env->mtrr_fixed, 0, sizeof(env->mtrr_fixed));
>
> env->interrupt_injected = -1;
> - env->exception_injected = -1;
> + env->exception_nr = -1;
> + env->exception_pending = 0;
> + env->exception_injected = 0;
Note: I the patch-series I will submit I will add here:
+ env->exception_has_payload = false;
+ env->exception_payload = 0;
> env->nmi_injected = false;
> #if !defined(CONFIG_USER_ONLY)
> /* We hard-wire the BSP to the first CPU. */
> @@ -5173,12 +5175,6 @@ static int x86_cpu_filter_features(X86CPU *cpu)
> return rv;
> }
>
> -#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
> - (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
> - (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> -#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> - (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> - (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> {
> CPUState *cs = CPU(dev);
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index bd06523a53..bbeb7a9521 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -729,6 +729,13 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>
> #define CPUID_VENDOR_HYGON "HygonGenuine"
>
> +#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
> + (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
> + (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> +#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> + (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> + (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> +
> #define CPUID_MWAIT_IBE (1U << 1) /* Interrupts can exit capability */
> #define CPUID_MWAIT_EMX (1U << 0) /* enumeration supported */
>
> @@ -1332,10 +1339,14 @@ typedef struct CPUX86State {
>
> /* For KVM */
> uint32_t mp_state;
> - int32_t exception_injected;
> + int32_t exception_nr;
> int32_t interrupt_injected;
> uint8_t soft_interrupt;
> + uint8_t exception_pending;
> + uint8_t exception_injected;
> uint8_t has_error_code;
> + uint8_t exception_has_payload;
> + uint64_t exception_payload;
> uint32_t ins_len;
> uint32_t sipi_vector;
> bool tsc_valid;
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index 2751c8125c..dc4bb63536 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -605,7 +605,9 @@ static void hvf_store_events(CPUState *cpu, uint32_t ins_len, uint64_t idtvec_in
> X86CPU *x86_cpu = X86_CPU(cpu);
> CPUX86State *env = &x86_cpu->env;
>
> - env->exception_injected = -1;
> + env->exception_nr = -1;
> + env->exception_pending = 0;
> + env->exception_injected = 0;
> env->interrupt_injected = -1;
> env->nmi_injected = false;
> if (idtvec_info & VMCS_IDT_VEC_VALID) {
> @@ -619,7 +621,8 @@ static void hvf_store_events(CPUState *cpu, uint32_t ins_len, uint64_t idtvec_in
> break;
> case VMCS_IDT_VEC_HWEXCEPTION:
> case VMCS_IDT_VEC_SWEXCEPTION:
> - env->exception_injected = idtvec_info & VMCS_IDT_VEC_VECNUM;
> + env->exception_nr = idtvec_info & VMCS_IDT_VEC_VECNUM;
> + env->exception_injected = 1;
> break;
> case VMCS_IDT_VEC_PRIV_SWEXCEPTION:
> default:
> @@ -912,7 +915,8 @@ int hvf_vcpu_exec(CPUState *cpu)
> macvm_set_rip(cpu, rip + ins_len);
> break;
> case VMX_REASON_VMCALL:
> - env->exception_injected = EXCP0D_GPF;
> + env->exception_nr = EXCP0D_GPF;
> + env->exception_injected = 1;
> env->has_error_code = true;
> env->error_code = 0;
> break;
> diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
> index df8e946fbc..e0ea02d631 100644
> --- a/target/i386/hvf/x86hvf.c
> +++ b/target/i386/hvf/x86hvf.c
> @@ -362,8 +362,8 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
> if (env->interrupt_injected != -1) {
> vector = env->interrupt_injected;
> intr_type = VMCS_INTR_T_SWINTR;
> - } else if (env->exception_injected != -1) {
> - vector = env->exception_injected;
> + } else if (env->exception_nr != -1) {
> + vector = env->exception_nr;
> if (vector == EXCP03_INT3 || vector == EXCP04_INTO) {
> intr_type = VMCS_INTR_T_SWEXCEPTION;
> } else {
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 53f95b02a0..dca76830ec 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -104,6 +104,7 @@ static uint32_t num_architectural_pmu_fixed_counters;
> static int has_xsave;
> static int has_xcrs;
> static int has_pit_state2;
> +static int has_exception_payload;
>
> static bool has_msr_mcg_ext_ctl;
>
> @@ -584,15 +585,51 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> /* Hope we are lucky for AO MCE */
> }
>
> +static void kvm_reset_exception(CPUX86State *env)
> +{
> + env->exception_nr = -1;
> + env->exception_pending = 0;
> + env->exception_injected = 0;
Note: In the patch-series I will submit I will add here:
+ env->exception_has_payload = false;
+ env->exception_payload = 0;
> +}
> +
> +static void kvm_queue_exception(CPUX86State *env,
> + int32_t exception_nr,
> + uint8_t exception_has_payload,
> + uint64_t exception_payload)
> +{
> + assert(env->exception_nr == -1);
> + assert(!env->exception_pending);
> + assert(!env->exception_injected);
Note: In the patch-series I will submit I will add here:
+ assert(!env->exception_has_payload);
> +
> + env->exception_nr = exception_nr;
> +
> + if (has_exception_payload) {
> + env->exception_pending = 1;
> +
> + env->exception_has_payload = exception_has_payload;
> + env->exception_payload = exception_payload;
> + } else {
> + env->exception_injected = 1;
> +
> + if (exception_has_payload) {
> + if (exception_nr == EXCP01_DB) {
> + env->dr[6] = exception_payload;
> + } else if (exception_nr == EXCP0E_PAGE) {
> + env->cr[2] = exception_payload;
> + }
> + }
Note: In the patch-series I will submit, I have changed this
to verify that #DB & #PF always have env->exception_has_payload set to true
and other cases have it set to false.
> + }
> +}
> +
> static int kvm_inject_mce_oldstyle(X86CPU *cpu)
> {
> CPUX86State *env = &cpu->env;
>
> - if (!kvm_has_vcpu_events() && env->exception_injected == EXCP12_MCHK) {
> + if (!kvm_has_vcpu_events() && env->exception_nr == EXCP12_MCHK) {
> unsigned int bank, bank_num = env->mcg_cap & 0xff;
> struct kvm_x86_mce mce;
>
> - env->exception_injected = -1;
> + kvm_reset_exception(env);
>
> /*
> * There must be at least one bank in use if an MCE is pending.
> @@ -1573,6 +1610,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>
> hv_vpindex_settable = kvm_check_extension(s, KVM_CAP_HYPERV_VP_INDEX);
>
> + has_exception_payload = kvm_check_extension(s, KVM_CAP_EXCEPTION_PAYLOAD);
> + if (has_exception_payload) {
> + ret = kvm_vm_enable_cap(s, KVM_CAP_EXCEPTION_PAYLOAD, 0, true);
> + if (ret < 0) {
> + error_report("kvm: Failed to enable exception payload cap: %s",
> + strerror(-ret));
> + return ret;
> + }
> + }
> +
> ret = kvm_get_supported_msrs(s);
> if (ret < 0) {
> return ret;
> @@ -2877,8 +2924,16 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
> return 0;
> }
>
> - events.exception.injected = (env->exception_injected >= 0);
> - events.exception.nr = env->exception_injected;
> + events.flags = 0;
> +
> + if (has_exception_payload) {
> + events.flags |= KVM_VCPUEVENT_VALID_PAYLOAD;
> + events.exception.pending = env->exception_pending;
> + events.exception_has_payload = env->exception_has_payload;
> + events.exception_payload = env->exception_payload;
> + }
> + events.exception.nr = env->exception_nr;
> + events.exception.injected = env->exception_injected;
> events.exception.has_error_code = env->has_error_code;
> events.exception.error_code = env->error_code;
>
> @@ -2891,7 +2946,6 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
> events.nmi.masked = !!(env->hflags2 & HF2_NMI_MASK);
>
> events.sipi_vector = env->sipi_vector;
> - events.flags = 0;
>
> if (has_msr_smbase) {
> events.smi.smm = !!(env->hflags & HF_SMM_MASK);
> @@ -2941,8 +2995,19 @@ static int kvm_get_vcpu_events(X86CPU *cpu)
> if (ret < 0) {
> return ret;
> }
> - env->exception_injected =
> - events.exception.injected ? events.exception.nr : -1;
> +
> + if (events.flags & KVM_VCPUEVENT_VALID_PAYLOAD) {
> + env->exception_pending = events.exception.pending;
> + env->exception_has_payload = events.exception_has_payload;
> + env->exception_payload = events.exception_payload;
> + } else {
> + env->exception_pending = 0;
> + env->exception_has_payload = false;
> + }
> + env->exception_injected = events.exception.injected;
> + env->exception_nr =
> + (env->exception_pending || env->exception_injected) ?
> + events.exception.nr : -1;
> env->has_error_code = events.exception.has_error_code;
> env->error_code = events.exception.error_code;
>
> @@ -2994,12 +3059,12 @@ static int kvm_guest_debug_workarounds(X86CPU *cpu)
> unsigned long reinject_trap = 0;
>
> if (!kvm_has_vcpu_events()) {
> - if (env->exception_injected == EXCP01_DB) {
> + if (env->exception_nr == EXCP01_DB) {
> reinject_trap = KVM_GUESTDBG_INJECT_DB;
> } else if (env->exception_injected == EXCP03_INT3) {
> reinject_trap = KVM_GUESTDBG_INJECT_BP;
> }
> - env->exception_injected = -1;
> + kvm_reset_exception(env);
> }
>
> /*
> @@ -3320,13 +3385,13 @@ int kvm_arch_process_async_events(CPUState *cs)
>
> kvm_cpu_synchronize_state(cs);
>
> - if (env->exception_injected == EXCP08_DBLE) {
> + if (env->exception_nr == EXCP08_DBLE) {
> /* this means triple fault */
> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> cs->exit_request = 1;
> return 0;
> }
> - env->exception_injected = EXCP12_MCHK;
> + kvm_queue_exception(env, EXCP12_MCHK, 0, 0);
> env->has_error_code = 0;
>
> cs->halted = 0;
> @@ -3541,14 +3606,12 @@ static int kvm_handle_debug(X86CPU *cpu,
> }
> if (ret == 0) {
> cpu_synchronize_state(cs);
> - assert(env->exception_injected == -1);
> + assert(env->exception_nr == -1);
>
> /* pass to guest */
> - env->exception_injected = arch_info->exception;
> + kvm_queue_exception(env, arch_info->exception,
> + EXCP01_DB, arch_info->dr6);
There is a bug here I will fix in my patch-series:
Third argument should be (arch_info->exception == EXCP01_DB).
> env->has_error_code = 0;
> - if (arch_info->exception == EXCP01_DB) {
> - env->dr[6] = arch_info->dr6;
> - }
> }
>
> return ret;
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 225b5d433b..41460be54b 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -199,6 +199,21 @@ static const VMStateDescription vmstate_fpreg = {
> }
> };
>
> +static bool is_vmx_enabled(CPUX86State *env)
> +{
> + return (IS_INTEL_CPU(env) && (env->cr[4] & CR4_VMXE_MASK));
> +}
> +
> +static bool is_svm_enabled(CPUX86State *env)
> +{
> + return (IS_AMD_CPU(env) && (env->efer & MSR_EFER_SVME));
> +}
> +
> +static bool is_nested_virt_enabled(CPUX86State *env)
> +{
> + return (is_vmx_enabled(env) || is_svm_enabled(env));
> +}
I have later realised that this nested_virt_enabled() function is not enough to determine if nested_state is required to be sent.
This is because it may be that vCPU is running L2 but have momentarily entered SMM due to SMI.
In this case, CR4 & MSR_EFER are saved in SMRAM and are set to 0 on entering to SMM.
This means that in case (env->hflags & HF_SMM_MASK), we theoretically should have read saved CR4 & MSR_EFER from SMRAM.
However, because we cannot reference guest memory at this point (Not valid in case we migrate guest in post-copy), I should change
code to assume that in case (env->hflags & HF_SMM_MASK), we need to assume that nested-state is needed.
This should break backwards-compatability migration only in very rare cases and therefore I think it should be sufficient.
Any objections to this idea?
> +
> static int cpu_pre_save(void *opaque)
> {
In my next patch-series I have added logic to cpu_pre_save that converts
a pending exception to an injected exception in case there is an exception pending
and nested-virtualization is not enabled. During this conversion, I also make sure
to apply the exception payload to the respective vCPU register (i.e. CR2 in case of #PF
or DR6 in case of #DB).
I have realised this is required because otherwise exception-info VMState subsection
will be deemed not needed and exception-payload will be lost.
Something like this:
+ /*
+ * When vCPU is running L2 and exception is still pending,
+ * it can potentially be intercepted by L1 hypervisor.
+ * In contrast to an injected exception which cannot be
+ * intercepted anymore.
+ *
+ * Furthermore, when a L2 exception is intercepted by L1
+ * hypervisor, it's exception payload (CR2/DR6 on #PF/#DB)
+ * should not be set yet in the respective vCPU register.
+ * Thus, in case an exception is pending, it is
+ * important to save the exception payload seperately.
+ *
+ * Therefore, if an exception is not in a pending state
+ * or guest is not running a nested hypervisor, it is
+ * not important to distinguish between a pending and
+ * injected exception and we don't need to store seperately
+ * the exception payload.
+ *
+ * In order to preserve better backwards-compatabile migration,
+ * convert a pending exception to an injected exception in
+ * case it is not important to distingiush between them
+ * as described above.
+ */
+ if (!is_nested_virt_enabled(env) && env->exception_pending) {
+ env->exception_pending = 0;
+ env->exception_injected = 1;
+
+ if (env->exception_nr == EXCP01_DB) {
+ assert(env->exception_has_payload);
+ env->dr[6] = env->exception_payload;
+ } else if (env->exception_nr == EXCP0E_PAGE) {
+ assert(env->exception_has_payload);
+ env->cr[2] = env->exception_payload;
+ } else {
+ assert(!env->exception_has_payload);
+ }
+ }
+
> X86CPU *cpu = opaque;
> @@ -278,6 +293,23 @@ static int cpu_post_load(void *opaque, int version_id)
> env->hflags &= ~HF_CPL_MASK;
> env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
>
> + /*
> + * There are cases that we can get valid exception_nr with both
> + * exception_pending and exception_clear being cleared. This can happen in
> + * one of the following scenarios:
> + * 1) Source is older QEMU without KVM_CAP_EXCEPTION_PAYLOAD support.
> + * 2) Source is running on kernel without KVM_CAP_EXCEPTION_PAYLOAD support.
> + * 3) "cpu/exception_info" subsection not sent because there is no exception
> + * pending or guest wasn't running L2.
> + *
> + * In those cases, we can just deduce that a valid exception_nr means
> + * we can treat the exception as already injected.
> + */
> + if ((env->exception_nr != -1) &&
> + !env->exception_pending && !env->exception_injected) {
> + env->exception_injected = 1;
> + }
> +
> env->fpstt = (env->fpus_vmstate >> 11) & 7;
> env->fpus = env->fpus_vmstate & ~0x3800;
> env->fptag_vmstate ^= 0xff;
> @@ -323,6 +355,32 @@ static bool steal_time_msr_needed(void *opaque)
> return cpu->env.steal_time_msr != 0;
> }
>
> +static bool exception_info_needed(void *opaque)
> +{
> + X86CPU *cpu = opaque;
> +
> + /*
> + * Differenting between pending and injected exceptions
> + * is only important when running L2.
> + */
> + return (cpu->env.exception_pending &&
> + is_nested_virt_enabled(&cpu->env));
> +}
> +
> +static const VMStateDescription vmstate_exception_info = {
> + .name = "cpu/exception_info",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = exception_info_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(env.exception_pending, X86CPU),
> + VMSTATE_UINT8(env.exception_injected, X86CPU),
> + VMSTATE_UINT8(env.exception_has_payload, X86CPU),
> + VMSTATE_UINT64(env.exception_payload, X86CPU),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_steal_time_msr = {
> .name = "cpu/steal_time_msr",
> .version_id = 1,
> @@ -1035,7 +1093,7 @@ VMStateDescription vmstate_x86_cpu = {
> VMSTATE_INT32(env.interrupt_injected, X86CPU),
> VMSTATE_UINT32(env.mp_state, X86CPU),
> VMSTATE_UINT64(env.tsc, X86CPU),
> - VMSTATE_INT32(env.exception_injected, X86CPU),
> + VMSTATE_INT32(env.exception_nr, X86CPU),
> VMSTATE_UINT8(env.soft_interrupt, X86CPU),
> VMSTATE_UINT8(env.nmi_injected, X86CPU),
> VMSTATE_UINT8(env.nmi_pending, X86CPU),
> @@ -1059,6 +1117,7 @@ VMStateDescription vmstate_x86_cpu = {
> /* The above list is not sorted /wrt version numbers, watch out! */
> },
> .subsections = (const VMStateDescription*[]) {
> + &vmstate_exception_info,
> &vmstate_async_pf_msr,
> &vmstate_pv_eoi_msr,
> &vmstate_steal_time_msr,
> --
> 2.21.0
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD
2019-06-15 0:57 ` Liran Alon
@ 2019-06-16 12:38 ` Liran Alon
2019-06-17 11:34 ` Liran Alon
0 siblings, 1 reply; 19+ messages in thread
From: Liran Alon @ 2019-06-16 12:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Maran Wilson, qemu-devel, Nikita Leshenko
> On 15 Jun 2019, at 3:57, Liran Alon <liran.alon@oracle.com> wrote:
>
>> On 15 Jun 2019, at 3:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> From: Liran Alon <liran.alon@oracle.com>
>>
>> +static bool is_vmx_enabled(CPUX86State *env)
>> +{
>> + return (IS_INTEL_CPU(env) && (env->cr[4] & CR4_VMXE_MASK));
>> +}
>> +
>> +static bool is_svm_enabled(CPUX86State *env)
>> +{
>> + return (IS_AMD_CPU(env) && (env->efer & MSR_EFER_SVME));
>> +}
>> +
>> +static bool is_nested_virt_enabled(CPUX86State *env)
>> +{
>> + return (is_vmx_enabled(env) || is_svm_enabled(env));
>> +}
>
> I have later realised that this nested_virt_enabled() function is not enough to determine if nested_state is required to be sent.
> This is because it may be that vCPU is running L2 but have momentarily entered SMM due to SMI.
> In this case, CR4 & MSR_EFER are saved in SMRAM and are set to 0 on entering to SMM.
> This means that in case (env->hflags & HF_SMM_MASK), we theoretically should have read saved CR4 & MSR_EFER from SMRAM.
> However, because we cannot reference guest memory at this point (Not valid in case we migrate guest in post-copy), I should change
> code to assume that in case (env->hflags & HF_SMM_MASK), we need to assume that nested-state is needed.
> This should break backwards-compatability migration only in very rare cases and therefore I think it should be sufficient.
> Any objections to this idea?
>
Actually, this is even worse than I originally thought.
Even in case guest is not currently in SMM mode, if it’s in VMX non-root mode, the CR4 read here is L2 CR4. Not L1 CR4.
Therefore, CR4.VMXE doesn’t necessarily indicate if guest have nested-virtualization enabled. Same is true for MSR_EFER in case of SVM.
Putting this all together, in case kernel doesn’t support extracting nested-state, there is no decent way to know if guest is running nested-virtualization.
Which means that in theory we always need to fail migration in case kernel doesn’t support KVM_CAP_NESTED_STATE or KVM_CAP_EXCEPTION_PAYLOAD
and vCPU is exposed with VMX/SVM capability.
I can condition this behaviour with a flag that can be manipulated using QMP to allow user to indicate it wishes to migrate guest anyway in this case.
This however bring me back to the entire discussion I had with Dr. David Alan Gilbert on migration backwards compatibility in general and the fact that I believe
we should have a generic QMP command which allows to provide list of VMState subsections that can be ignored in migration…
See: https://www.mail-archive.com/qemu-devel@nongnu.org/msg622274.html
Paolo, What are your thoughts on how I would proceed with this?
-Liran
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD
2019-06-16 12:38 ` Liran Alon
@ 2019-06-17 11:34 ` Liran Alon
2019-06-17 17:27 ` Paolo Bonzini
0 siblings, 1 reply; 19+ messages in thread
From: Liran Alon @ 2019-06-17 11:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Maran Wilson, qemu-devel, Nikita Leshenko
> On 16 Jun 2019, at 15:38, Liran Alon <liran.alon@oracle.com> wrote:
>
>
>
>> On 15 Jun 2019, at 3:57, Liran Alon <liran.alon@oracle.com> wrote:
>>
>>> On 15 Jun 2019, at 3:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> From: Liran Alon <liran.alon@oracle.com>
>>>
>>> +static bool is_vmx_enabled(CPUX86State *env)
>>> +{
>>> + return (IS_INTEL_CPU(env) && (env->cr[4] & CR4_VMXE_MASK));
>>> +}
>>> +
>>> +static bool is_svm_enabled(CPUX86State *env)
>>> +{
>>> + return (IS_AMD_CPU(env) && (env->efer & MSR_EFER_SVME));
>>> +}
>>> +
>>> +static bool is_nested_virt_enabled(CPUX86State *env)
>>> +{
>>> + return (is_vmx_enabled(env) || is_svm_enabled(env));
>>> +}
>>
>> I have later realised that this nested_virt_enabled() function is not enough to determine if nested_state is required to be sent.
>> This is because it may be that vCPU is running L2 but have momentarily entered SMM due to SMI.
>> In this case, CR4 & MSR_EFER are saved in SMRAM and are set to 0 on entering to SMM.
>> This means that in case (env->hflags & HF_SMM_MASK), we theoretically should have read saved CR4 & MSR_EFER from SMRAM.
>> However, because we cannot reference guest memory at this point (Not valid in case we migrate guest in post-copy), I should change
>> code to assume that in case (env->hflags & HF_SMM_MASK), we need to assume that nested-state is needed.
>> This should break backwards-compatability migration only in very rare cases and therefore I think it should be sufficient.
>> Any objections to this idea?
>>
>
> Actually, this is even worse than I originally thought.
> Even in case guest is not currently in SMM mode, if it’s in VMX non-root mode, the CR4 read here is L2 CR4. Not L1 CR4.
> Therefore, CR4.VMXE doesn’t necessarily indicate if guest have nested-virtualization enabled. Same is true for MSR_EFER in case of SVM.
>
> Putting this all together, in case kernel doesn’t support extracting nested-state, there is no decent way to know if guest is running nested-virtualization.
> Which means that in theory we always need to fail migration in case kernel doesn’t support KVM_CAP_NESTED_STATE or KVM_CAP_EXCEPTION_PAYLOAD
> and vCPU is exposed with VMX/SVM capability.
>
> I can condition this behaviour with a flag that can be manipulated using QMP to allow user to indicate it wishes to migrate guest anyway in this case.
> This however bring me back to the entire discussion I had with Dr. David Alan Gilbert on migration backwards compatibility in general and the fact that I believe
> we should have a generic QMP command which allows to provide list of VMState subsections that can be ignored in migration…
> See: https://www.mail-archive.com/qemu-devel@nongnu.org/msg622274.html
>
> Paolo, What are your thoughts on how I would proceed with this?
>
> -Liran
>
Paolo, can you also elaborate what was your original intent in this QEMU commit you made:
f8dc4c645ec2 ("target/i386: rename HF_SVMI_MASK to HF_GUEST_MASK")
How did you expect this flag to be used in nVMX migration?
Currently the HF_GUEST_MASK flag in KVM that is set in vcpu->arch.hflags is never propagated to userspace.
Did you expect to set HF_GUEST_MASK in QEMU’s hflag based on nested_state returned by KVM_GET_NESTED_STATE ioctl?
What is the value of this hflag in QEMU given that we have all the information we need in env->nested_state?
(E.g. We can deduce vCPU is in VMX non-root mode by testing for (env->nested_state.flags & KVM_STATE_NESTED_GUEST_MODE)).
-Liran
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD
2019-06-17 11:34 ` Liran Alon
@ 2019-06-17 17:27 ` Paolo Bonzini
2019-06-18 22:38 ` Maran Wilson
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-17 17:27 UTC (permalink / raw)
To: Liran Alon; +Cc: Maran Wilson, qemu-devel, Nikita Leshenko
On 17/06/19 13:34, Liran Alon wrote:
> Putting this all together, in case kernel doesn’t support extracting
> nested-state, there is no decent way to know if guest is running
> nested-virtualization. Which means that in theory we always need to
> fail migration in case kernel doesn’t support KVM_CAP_NESTED_STATE or
> KVM_CAP_EXCEPTION_PAYLOAD and vCPU is exposed with VMX/SVM
> capability.
For VMX this would be okay because we had a blocker before this series,
and this wouldn't be any worse.
For SVM we can ignore the case and fix it when we have
KVM_CAP_NESTED_STATE, as again that wouldn't be any worse.
Paolo
> I can condition this behaviour with a flag that can be manipulated
> using QMP to allow user to indicate it wishes to migrate guest anyway
> in this case. This however bring me back to the entire discussion I
> had with Dr. David Alan Gilbert on migration backwards compatibility
> in general and the fact that I believe we should have a generic QMP
> command which allows to provide list of VMState subsections that can
> be ignored in migration… See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg622274.html
>
> Paolo, What are your thoughts on how I would proceed with this?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD
2019-06-17 17:27 ` Paolo Bonzini
@ 2019-06-18 22:38 ` Maran Wilson
0 siblings, 0 replies; 19+ messages in thread
From: Maran Wilson @ 2019-06-18 22:38 UTC (permalink / raw)
To: Paolo Bonzini, Liran Alon; +Cc: qemu-devel, Nikita Leshenko
On 6/17/2019 10:27 AM, Paolo Bonzini wrote:
> On 17/06/19 13:34, Liran Alon wrote:
>> Putting this all together, in case kernel doesn’t support extracting
>> nested-state, there is no decent way to know if guest is running
>> nested-virtualization. Which means that in theory we always need to
>> fail migration in case kernel doesn’t support KVM_CAP_NESTED_STATE or
>> KVM_CAP_EXCEPTION_PAYLOAD and vCPU is exposed with VMX/SVM
>> capability.
> For VMX this would be okay because we had a blocker before this series,
> and this wouldn't be any worse.
I agree it shouldn't be a gating issue for this patch series, but I'd
hate to see this discussion thread die off.
I'm still pretty interested in hearing whether anyone has any good ideas
for how to conclusively determine whether a given L1 VM has created a
nested L2 or not when the host is running an older Kernel that doesn't
support KVM_CAP_NESTED_STATE. That would be a very useful capability,
especially for CSP use cases. If anyone has any suggestions about where
to look, I don't mind spending some time digging into it and possibly
testing out a few ideas. Again, separate from this particular patch
series. So far I've been drawing a blank after Liran pointed out that
corner case problems associated with env->cr[4] & CR4_VMXE_MASK.
Thanks,
-Maran
> For SVM we can ignore the case and fix it when we have
> KVM_CAP_NESTED_STATE, as again that wouldn't be any worse.
>
> Paolo
>
>> I can condition this behaviour with a flag that can be manipulated
>> using QMP to allow user to indicate it wishes to migrate guest anyway
>> in this case. This however bring me back to the entire discussion I
>> had with Dr. David Alan Gilbert on migration backwards compatibility
>> in general and the fact that I believe we should have a generic QMP
>> command which allows to provide list of VMState subsections that can
>> be ignored in migration… See:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg622274.html
>>
>> Paolo, What are your thoughts on how I would proceed with this?
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
` (2 preceding siblings ...)
2019-06-15 0:42 ` [Qemu-devel] [PATCH 3/7] KVM: i386: Add support for KVM_CAP_EXCEPTION_PAYLOAD Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
2019-06-16 8:29 ` Liran Alon
2019-06-15 0:42 ` [Qemu-devel] [PATCH 5/7] vmstate: Add support for kernel integer types Paolo Bonzini
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
This patch improves the KVM_GET/SET_NESTED_STATE structs by detailing
the format of VMX nested state in a struct. The VMX nested state is
accessible through struct kvm_vmx_nested_state though, to avoid
changing the size of the structs, it has to be accessed as "vmx.data[0]"
rather than just "vmx.data".
Also, the values of the "format" field are defined as macros. This
patch should be sent to Linus very shortly.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
| 11 +++++++++++
1 file changed, 11 insertions(+)
--git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index 7a0e64ccd6..06b8727a3b 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -383,6 +383,9 @@ struct kvm_sync_regs {
#define KVM_X86_QUIRK_LAPIC_MMIO_HOLE (1 << 2)
#define KVM_X86_QUIRK_OUT_7E_INC_RIP (1 << 3)
+#define KVM_STATE_NESTED_FORMAT_VMX 0
+#define KVM_STATE_NESTED_FORMAT_SVM 1
+
#define KVM_STATE_NESTED_GUEST_MODE 0x00000001
#define KVM_STATE_NESTED_RUN_PENDING 0x00000002
#define KVM_STATE_NESTED_EVMCS 0x00000004
@@ -390,6 +393,11 @@ struct kvm_sync_regs {
#define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001
#define KVM_STATE_NESTED_SMM_VMXON 0x00000002
+struct kvm_vmx_nested_state_data {
+ __u8 vmcs12[0x1000];
+ __u8 shadow_vmcs12[0x1000];
+};
+
struct kvm_vmx_nested_state {
__u64 vmxon_pa;
__u64 vmcs_pa;
@@ -397,6 +405,9 @@ struct kvm_vmx_nested_state {
struct {
__u16 flags;
} smm;
+
+ __u8 pad[120 - 18];
+ struct kvm_vmx_nested_state_data data[0];
};
/* for KVM_CAP_NESTED_STATE */
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs
2019-06-15 0:42 ` [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs Paolo Bonzini
@ 2019-06-16 8:29 ` Liran Alon
2019-06-17 17:32 ` Paolo Bonzini
0 siblings, 1 reply; 19+ messages in thread
From: Liran Alon @ 2019-06-16 8:29 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, nikita.leshchenko
> On 15 Jun 2019, at 3:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> This patch improves the KVM_GET/SET_NESTED_STATE structs by detailing
> the format of VMX nested state in a struct. The VMX nested state is
> accessible through struct kvm_vmx_nested_state though, to avoid
> changing the size of the structs, it has to be accessed as "vmx.data[0]"
> rather than just "vmx.data".
>
> Also, the values of the "format" field are defined as macros. This
> patch should be sent to Linus very shortly.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> linux-headers/asm-x86/kvm.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
> index 7a0e64ccd6..06b8727a3b 100644
> --- a/linux-headers/asm-x86/kvm.h
> +++ b/linux-headers/asm-x86/kvm.h
> @@ -383,6 +383,9 @@ struct kvm_sync_regs {
> #define KVM_X86_QUIRK_LAPIC_MMIO_HOLE (1 << 2)
> #define KVM_X86_QUIRK_OUT_7E_INC_RIP (1 << 3)
>
> +#define KVM_STATE_NESTED_FORMAT_VMX 0
> +#define KVM_STATE_NESTED_FORMAT_SVM 1
> +
> #define KVM_STATE_NESTED_GUEST_MODE 0x00000001
> #define KVM_STATE_NESTED_RUN_PENDING 0x00000002
> #define KVM_STATE_NESTED_EVMCS 0x00000004
> @@ -390,6 +393,11 @@ struct kvm_sync_regs {
> #define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001
> #define KVM_STATE_NESTED_SMM_VMXON 0x00000002
>
> +struct kvm_vmx_nested_state_data {
> + __u8 vmcs12[0x1000];
> + __u8 shadow_vmcs12[0x1000];
> +};
Do you think we should replace this 0x1000 with VMCS12_SIZE?
> +
> struct kvm_vmx_nested_state {
> __u64 vmxon_pa;
> __u64 vmcs_pa;
> @@ -397,6 +405,9 @@ struct kvm_vmx_nested_state {
> struct {
> __u16 flags;
> } smm;
> +
> + __u8 pad[120 - 18];
> + struct kvm_vmx_nested_state_data data[0];
> };
I don’t like this pad[] thing.
It creates weird dependency between the padding in kvm_nested_state and this one.
Also, it doesn’t separate nicely between header & data regions.
What do you think on the following alternative patch?
(Note that it should still preserve kvm_nested_state struct size)
-struct kvm_vmx_nested_state {
+struct kvm_vmx_nested_state_data {
+ __u8 vmcs12[0x1000];
+ __u8 shadow_vmcs12[0x1000];
+};
+
+struct kvm_vmx_nested_state_hdr {
__u64 vmxon_pa;
- __u64 vmcs_pa;
+ __u64 vmcs12_pa;
struct {
__u16 flags;
} smm;
};
+struct kvm_svm_nested_state_data {
+ /* TODO: Implement */
+};
+
+struct kvm_svm_nested_state_hdr {
+ /* TODO: Implement */
+};
+
/* for KVM_CAP_NESTED_STATE */
struct kvm_nested_state {
- /* KVM_STATE_* flags */
__u16 flags;
-
- /* 0 for VMX, 1 for SVM. */
__u16 format;
-
- /* 128 for SVM, 128 + VMCS size for VMX. */
__u32 size;
union {
- /* VMXON, VMCS */
- struct kvm_vmx_nested_state vmx;
+ struct kvm_vmx_nested_state_hdr vmx;
+ struct kvm_svm_nested_state_hdr svm;
/* Pad the header to 128 bytes. */
__u8 pad[120];
- };
+ } hdr;
- __u8 data[0];
+ /*
+ * Define data region as 0 bytes to preserve backwards-compatability
+ * to old definition of kvm_nested_state in order to avoid changing
+ * KVM_{GET,PUT}_NESTED_STATE ioctl values.
+ */
+ union {
+ struct kvm_vmx_nested_state_data vmx[0];
+ struct kvm_svm_nested_state_data svm[0];
+ } data;
};
I think this is cleaner.
-Liran
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs
2019-06-16 8:29 ` Liran Alon
@ 2019-06-17 17:32 ` Paolo Bonzini
2019-06-17 17:44 ` Liran Alon
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-17 17:32 UTC (permalink / raw)
To: Liran Alon; +Cc: qemu-devel, nikita.leshchenko
On 16/06/19 10:29, Liran Alon wrote:
>
> I think this is cleaner.
>
> -Liran
Yes, it is. I'll post it to kvm@vger.kernel.org. Are you going to send
v2 of this series or shall I?
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs
2019-06-17 17:32 ` Paolo Bonzini
@ 2019-06-17 17:44 ` Liran Alon
0 siblings, 0 replies; 19+ messages in thread
From: Liran Alon @ 2019-06-17 17:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, nikita.leshchenko
> On 17 Jun 2019, at 20:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 16/06/19 10:29, Liran Alon wrote:
>>
>> I think this is cleaner.
>>
>> -Liran
>
> Yes, it is. I'll post it to kvm@vger.kernel.org. Are you going to send
> v2 of this series or shall I?
>
> Paolo
The KVM patch is already submitted and I think you saw it.
I’m just about to send today a new v2 for the QEMU patches. It has additional changes.
-Liran
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 5/7] vmstate: Add support for kernel integer types
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
` (3 preceding siblings ...)
2019-06-15 0:42 ` [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
2019-06-15 0:42 ` [Qemu-devel] [PATCH 6/7] KVM: i386: Add support for save and restore nested state Paolo Bonzini
2019-06-15 0:42 ` [Qemu-devel] [PATCH 7/7] Revert "target/i386: kvm: add VMX migration blocker" Paolo Bonzini
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
From: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/migration/vmstate.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9224370ed5..a85424fb04 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -797,6 +797,15 @@ extern const VMStateInfo vmstate_info_qtailq;
#define VMSTATE_UINT64_V(_f, _s, _v) \
VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
+#define VMSTATE_U8_V(_f, _s, _v) \
+ VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint8, __u8)
+#define VMSTATE_U16_V(_f, _s, _v) \
+ VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16, __u16)
+#define VMSTATE_U32_V(_f, _s, _v) \
+ VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, __u32)
+#define VMSTATE_U64_V(_f, _s, _v) \
+ VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, __u64)
+
#define VMSTATE_BOOL(_f, _s) \
VMSTATE_BOOL_V(_f, _s, 0)
@@ -818,6 +827,15 @@ extern const VMStateInfo vmstate_info_qtailq;
#define VMSTATE_UINT64(_f, _s) \
VMSTATE_UINT64_V(_f, _s, 0)
+#define VMSTATE_U8(_f, _s) \
+ VMSTATE_U8_V(_f, _s, 0)
+#define VMSTATE_U16(_f, _s) \
+ VMSTATE_U16_V(_f, _s, 0)
+#define VMSTATE_U32(_f, _s) \
+ VMSTATE_U32_V(_f, _s, 0)
+#define VMSTATE_U64(_f, _s) \
+ VMSTATE_U64_V(_f, _s, 0)
+
#define VMSTATE_UINT8_EQUAL(_f, _s, _err_hint) \
VMSTATE_SINGLE_FULL(_f, _s, 0, 0, \
vmstate_info_uint8_equal, uint8_t, _err_hint)
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH 6/7] KVM: i386: Add support for save and restore nested state
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
` (4 preceding siblings ...)
2019-06-15 0:42 ` [Qemu-devel] [PATCH 5/7] vmstate: Add support for kernel integer types Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
2019-06-15 1:14 ` Liran Alon
2019-06-15 0:42 ` [Qemu-devel] [PATCH 7/7] Revert "target/i386: kvm: add VMX migration blocker" Paolo Bonzini
6 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
From: Liran Alon <liran.alon@oracle.com>
Kernel commit 8fcc4b5923af ("kvm: nVMX: Introduce KVM_CAP_NESTED_STATE")
introduced new IOCTLs to extract and restore KVM internal state used to
run a VM that is in VMX operation.
Utilize these IOCTLs to add support of migration of VMs which are
running nested hypervisors.
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
[Simplified subsection needed functions and computation of
kvm_min_nested_state_len(); adjusted for upstream kernel field
names; fixed !CONFIG_KVM compilation. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
accel/kvm/kvm-all.c | 8 ++
include/sysemu/kvm.h | 1 +
target/i386/cpu.h | 3 +
target/i386/kvm.c | 52 ++++++++++++
target/i386/machine.c | 182 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 246 insertions(+)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e4ac3386cb..e1c6c067e8 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -88,6 +88,7 @@ struct KVMState
#ifdef KVM_CAP_SET_GUEST_DEBUG
QTAILQ_HEAD(, kvm_sw_breakpoint) kvm_sw_breakpoints;
#endif
+ uint32_t max_nested_state_len;
int many_ioeventfds;
int intx_set_mask;
bool sync_mmu;
@@ -1677,6 +1678,8 @@ static int kvm_init(MachineState *ms)
s->debugregs = kvm_check_extension(s, KVM_CAP_DEBUGREGS);
#endif
+ s->max_nested_state_len = kvm_check_extension(s, KVM_CAP_NESTED_STATE);
+
#ifdef KVM_CAP_IRQ_ROUTING
kvm_direct_msi_allowed = (kvm_check_extension(s, KVM_CAP_SIGNAL_MSI) > 0);
#endif
@@ -2244,6 +2247,11 @@ int kvm_has_debugregs(void)
return kvm_state->debugregs;
}
+uint32_t kvm_max_nested_state_length(void)
+{
+ return kvm_state->max_nested_state_len;
+}
+
int kvm_has_many_ioeventfds(void)
{
if (!kvm_enabled()) {
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a6d1cd190f..5eb79b594c 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -210,6 +210,7 @@ bool kvm_has_sync_mmu(void);
int kvm_has_vcpu_events(void);
int kvm_has_robust_singlestep(void);
int kvm_has_debugregs(void);
+uint32_t kvm_max_nested_state_length(void);
int kvm_has_pit_state2(void);
int kvm_has_many_ioeventfds(void);
int kvm_has_gsi_routing(void);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index bbeb7a9521..550d397807 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1355,6 +1355,9 @@ typedef struct CPUX86State {
#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
void *xsave_buf;
#endif
+#if defined(CONFIG_KVM)
+ struct kvm_nested_state *nested_state;
+#endif
#if defined(CONFIG_HVF)
HVFX86EmulatorState *hvf_emul;
#endif
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index dca76830ec..d48fafa22b 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -968,6 +968,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
struct kvm_cpuid_entry2 *c;
uint32_t signature[3];
int kvm_base = KVM_CPUID_SIGNATURE;
+ uint32_t nested_state_len;
int r;
Error *local_err = NULL;
@@ -1368,6 +1369,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
if (has_xsave) {
env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
}
+
+ nested_state_len = kvm_max_nested_state_length();
+ if (nested_state_len > 0) {
+ assert(nested_state_len >= offsetof(struct kvm_nested_state, data));
+ env->nested_state = g_malloc0(nested_state_len);
+ }
+
cpu->kvm_msr_buf = g_malloc0(MSR_BUF_SIZE);
if (!(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
@@ -3125,6 +3133,41 @@ static int kvm_get_debugregs(X86CPU *cpu)
return 0;
}
+static int kvm_put_nested_state(X86CPU *cpu)
+{
+ CPUX86State *env = &cpu->env;
+ uint32_t nested_state_len = kvm_max_nested_state_length();
+
+ if (nested_state_len == 0) {
+ return 0;
+ }
+
+ assert(env->nested_state->size <= nested_state_len);
+ return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_NESTED_STATE, env->nested_state);
+}
+
+static int kvm_get_nested_state(X86CPU *cpu)
+{
+ CPUX86State *env = &cpu->env;
+ uint32_t nested_state_len = kvm_max_nested_state_length();
+
+ if (nested_state_len == 0) {
+ return 0;
+ }
+
+ /*
+ * It is possible that migration restored a smaller size into
+ * nested_state->size than what our kernel supports.
+ * We preserve migration origin nested_state->size for
+ * the call to KVM_SET_NESTED_STATE but wish that our next call
+ * to KVM_GET_NESTED_STATE will use the maximum size supported by
+ * the kernel we're running on.
+ */
+ env->nested_state->size = nested_state_len;
+
+ return kvm_vcpu_ioctl(CPU(cpu), KVM_GET_NESTED_STATE, env->nested_state);
+}
+
int kvm_arch_put_registers(CPUState *cpu, int level)
{
X86CPU *x86_cpu = X86_CPU(cpu);
@@ -3132,6 +3175,11 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
+ ret = kvm_put_nested_state(x86_cpu);
+ if (ret < 0) {
+ return ret;
+ }
+
if (level >= KVM_PUT_RESET_STATE) {
ret = kvm_put_msr_feature_control(x86_cpu);
if (ret < 0) {
@@ -3247,6 +3295,10 @@ int kvm_arch_get_registers(CPUState *cs)
if (ret < 0) {
goto out;
}
+ ret = kvm_get_nested_state(cpu);
+ if (ret < 0) {
+ goto out;
+ }
ret = 0;
out:
cpu_sync_bndcs_hflags(&cpu->env);
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 41460be54b..45dbae6054 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -246,6 +246,15 @@ static int cpu_pre_save(void *opaque)
env->segs[R_SS].flags &= ~(env->segs[R_SS].flags & DESC_DPL_MASK);
}
+#ifdef CONFIG_KVM
+ /* Verify we have nested virtualization state from kernel if required */
+ if (is_nested_virt_enabled(env) && !env->nested_state) {
+ error_report("Guest enabled nested virtualization but kernel "
+ "do not support saving nested state");
+ return -EINVAL;
+ }
+#endif
+
return 0;
}
@@ -909,6 +918,176 @@ static const VMStateDescription vmstate_tsc_khz = {
}
};
+#ifdef CONFIG_KVM
+static bool vmx_vmcs12_needed(void *opaque)
+{
+ struct kvm_nested_state *nested_state = opaque;
+ return (nested_state->size > offsetof(struct kvm_nested_state,
+ vmx.data[0].vmcs12));
+}
+
+static const VMStateDescription vmstate_vmx_vmcs12_state = {
+ .name = "cpu/kvm_nested_state/vmx/vmcs12",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmx_vmcs12_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_ARRAY(vmx.data[0].vmcs12,
+ struct kvm_nested_state, 0x1000),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static bool vmx_shadow_vmcs12_needed(void *opaque)
+{
+ struct kvm_nested_state *nested_state = opaque;
+ return (nested_state->size > offsetof(struct kvm_nested_state,
+ vmx.data[0].shadow_vmcs12));
+}
+
+static const VMStateDescription vmstate_vmx_shadow_vmcs12_state = {
+ .name = "cpu/kvm_nested_state/vmx/shadow_vmcs12",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmx_shadow_vmcs12_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_ARRAY(vmx.data[0].shadow_vmcs12,
+ struct kvm_nested_state, 0x1000),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static bool vmx_nested_state_needed(void *opaque)
+{
+ struct kvm_nested_state *nested_state = opaque;
+
+ return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
+ (nested_state->vmx.vmxon_pa != -1ull));
+}
+
+static const VMStateDescription vmstate_vmx_nested_state = {
+ .name = "cpu/kvm_nested_state/vmx",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmx_nested_state_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_U64(vmx.vmxon_pa, struct kvm_nested_state),
+ VMSTATE_U64(vmx.vmcs_pa, struct kvm_nested_state),
+ VMSTATE_U16(vmx.smm.flags, struct kvm_nested_state),
+ VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription*[]) {
+ &vmstate_vmx_vmcs12_state,
+ &vmstate_vmx_shadow_vmcs12_state,
+ NULL,
+ }
+};
+
+static bool svm_nested_state_needed(void *opaque)
+{
+ struct kvm_nested_state *nested_state = opaque;
+
+ return (nested_state->format == KVM_STATE_NESTED_FORMAT_SVM);
+}
+
+static const VMStateDescription vmstate_svm_nested_state = {
+ .name = "cpu/kvm_nested_state/svm",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = svm_nested_state_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static bool nested_state_needed(void *opaque)
+{
+ X86CPU *cpu = opaque;
+ CPUX86State *env = &cpu->env;
+
+ return (is_vmx_enabled(env) && vmx_nested_state_needed(env->nested_state)) ||
+ (is_svm_enabled(env) && svm_nested_state_needed(env->nested_state));
+}
+
+static int nested_state_post_load(void *opaque, int version_id)
+{
+ X86CPU *cpu = opaque;
+ CPUX86State *env = &cpu->env;
+ struct kvm_nested_state *nested_state = env->nested_state;
+ uint32_t min_nested_state_len = offsetof(struct kvm_nested_state, data);
+ uint32_t max_nested_state_len = kvm_max_nested_state_length();
+
+ /*
+ * If our kernel don't support setting nested state
+ * and we have received nested state from migration stream,
+ * we need to fail migration
+ */
+ if (max_nested_state_len == 0) {
+ error_report("Received nested state when kernel cannot restore it");
+ return -EINVAL;
+ }
+
+ /*
+ * Verify that the size of received nested_state struct
+ * at least cover required header and is not larger
+ * than the max size that our kernel support
+ */
+ if (nested_state->size < min_nested_state_len) {
+ error_report("Received nested state size less than min: "
+ "len=%d, min=%d",
+ nested_state->size, min_nested_state_len);
+ return -EINVAL;
+ }
+ if (nested_state->size > max_nested_state_len) {
+ error_report("Recieved unsupported nested state size: "
+ "nested_state->size=%d, max=%d",
+ nested_state->size, max_nested_state_len);
+ return -EINVAL;
+ }
+
+ /* Verify format is valid */
+ if ((nested_state->format != KVM_STATE_NESTED_FORMAT_VMX) &&
+ (nested_state->format != KVM_STATE_NESTED_FORMAT_SVM)) {
+ error_report("Received invalid nested state format: %d",
+ nested_state->format);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const VMStateDescription vmstate_kvm_nested_state = {
+ .name = "cpu/kvm_nested_state",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_U16(flags, struct kvm_nested_state),
+ VMSTATE_U16(format, struct kvm_nested_state),
+ VMSTATE_U32(size, struct kvm_nested_state),
+ VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription*[]) {
+ &vmstate_vmx_nested_state,
+ &vmstate_svm_nested_state,
+ NULL
+ }
+};
+
+static const VMStateDescription vmstate_nested_state = {
+ .name = "cpu/nested_state",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = nested_state_needed,
+ .post_load = nested_state_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT_POINTER(env.nested_state, X86CPU,
+ vmstate_kvm_nested_state,
+ struct kvm_nested_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+#endif
+
static bool mcg_ext_ctl_needed(void *opaque)
{
X86CPU *cpu = opaque;
@@ -1148,6 +1327,9 @@ VMStateDescription vmstate_x86_cpu = {
&vmstate_msr_intel_pt,
&vmstate_msr_virt_ssbd,
&vmstate_svm_npt,
+#ifdef CONFIG_KVM
+ &vmstate_nested_state,
+#endif
NULL
}
};
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] KVM: i386: Add support for save and restore nested state
2019-06-15 0:42 ` [Qemu-devel] [PATCH 6/7] KVM: i386: Add support for save and restore nested state Paolo Bonzini
@ 2019-06-15 1:14 ` Liran Alon
2019-06-17 17:31 ` Paolo Bonzini
0 siblings, 1 reply; 19+ messages in thread
From: Liran Alon @ 2019-06-15 1:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, nikita.leshchenko
> On 15 Jun 2019, at 3:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Liran Alon <liran.alon@oracle.com>
>
> Kernel commit 8fcc4b5923af ("kvm: nVMX: Introduce KVM_CAP_NESTED_STATE")
> introduced new IOCTLs to extract and restore KVM internal state used to
> run a VM that is in VMX operation.
>
> Utilize these IOCTLs to add support of migration of VMs which are
> running nested hypervisors.
>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> [Simplified subsection needed functions and computation of
> kvm_min_nested_state_len(); adjusted for upstream kernel field
> names; fixed !CONFIG_KVM compilation. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> accel/kvm/kvm-all.c | 8 ++
> include/sysemu/kvm.h | 1 +
> target/i386/cpu.h | 3 +
> target/i386/kvm.c | 52 ++++++++++++
> target/i386/machine.c | 182 ++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 246 insertions(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index e4ac3386cb..e1c6c067e8 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -88,6 +88,7 @@ struct KVMState
> #ifdef KVM_CAP_SET_GUEST_DEBUG
> QTAILQ_HEAD(, kvm_sw_breakpoint) kvm_sw_breakpoints;
> #endif
> + uint32_t max_nested_state_len;
Note: In my next patch-series I have changed this to be “int”.
> int many_ioeventfds;
> int intx_set_mask;
> bool sync_mmu;
> @@ -1677,6 +1678,8 @@ static int kvm_init(MachineState *ms)
> s->debugregs = kvm_check_extension(s, KVM_CAP_DEBUGREGS);
> #endif
>
> + s->max_nested_state_len = kvm_check_extension(s, KVM_CAP_NESTED_STATE);
> +
> #ifdef KVM_CAP_IRQ_ROUTING
> kvm_direct_msi_allowed = (kvm_check_extension(s, KVM_CAP_SIGNAL_MSI) > 0);
> #endif
> @@ -2244,6 +2247,11 @@ int kvm_has_debugregs(void)
> return kvm_state->debugregs;
> }
>
> +uint32_t kvm_max_nested_state_length(void)
> +{
> + return kvm_state->max_nested_state_len;
> +}
> +
> int kvm_has_many_ioeventfds(void)
> {
> if (!kvm_enabled()) {
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index a6d1cd190f..5eb79b594c 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -210,6 +210,7 @@ bool kvm_has_sync_mmu(void);
> int kvm_has_vcpu_events(void);
> int kvm_has_robust_singlestep(void);
> int kvm_has_debugregs(void);
> +uint32_t kvm_max_nested_state_length(void);
> int kvm_has_pit_state2(void);
> int kvm_has_many_ioeventfds(void);
> int kvm_has_gsi_routing(void);
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index bbeb7a9521..550d397807 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1355,6 +1355,9 @@ typedef struct CPUX86State {
> #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
> void *xsave_buf;
> #endif
> +#if defined(CONFIG_KVM)
> + struct kvm_nested_state *nested_state;
> +#endif
Nice catch regarding CONFIG_KVM. Thanks for that. :)
> #if defined(CONFIG_HVF)
> HVFX86EmulatorState *hvf_emul;
> #endif
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index dca76830ec..d48fafa22b 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -968,6 +968,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> struct kvm_cpuid_entry2 *c;
> uint32_t signature[3];
> int kvm_base = KVM_CPUID_SIGNATURE;
> + uint32_t nested_state_len;
> int r;
> Error *local_err = NULL;
>
> @@ -1368,6 +1369,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
> if (has_xsave) {
> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> }
> +
> + nested_state_len = kvm_max_nested_state_length();
> + if (nested_state_len > 0) {
> + assert(nested_state_len >= offsetof(struct kvm_nested_state, data));
> + env->nested_state = g_malloc0(nested_state_len);
Paolo, why have you removed setting “env->nested_state->size = max_nested_state_len;”?
In addition, in my next patch-series I also added the following code here which is required:
+ if (IS_INTEL_CPU(env)) {
+ struct kvm_vmx_nested_state_hdr *vmx_hdr =
+ &env->nested_state->hdr.vmx_hdr;
+
+ vmx_hdr->vmxon_pa = -1ull;
+ vmx_hdr->vmcs12_pa = -1ull;
+ }
> + }
> +
> cpu->kvm_msr_buf = g_malloc0(MSR_BUF_SIZE);
Note: In my next patch-series I have also added a new kvm_arch_destroy_vcpu() method which is called from kvm_destroy_vcpu().
Similar to how kvm_arch_init_vcpu() is called from kvm_init_vcpu().
I use it to free both cpu->kvm_msr_buf and env->nested_state.
>
> if (!(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
> @@ -3125,6 +3133,41 @@ static int kvm_get_debugregs(X86CPU *cpu)
> return 0;
> }
>
> +static int kvm_put_nested_state(X86CPU *cpu)
> +{
> + CPUX86State *env = &cpu->env;
> + uint32_t nested_state_len = kvm_max_nested_state_length();
> +
> + if (nested_state_len == 0) {
> + return 0;
> + }
> +
> + assert(env->nested_state->size <= nested_state_len);
> + return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_NESTED_STATE, env->nested_state);
> +}
> +
> +static int kvm_get_nested_state(X86CPU *cpu)
> +{
> + CPUX86State *env = &cpu->env;
> + uint32_t nested_state_len = kvm_max_nested_state_length();
> +
> + if (nested_state_len == 0) {
> + return 0;
> + }
> +
> + /*
> + * It is possible that migration restored a smaller size into
> + * nested_state->size than what our kernel supports.
> + * We preserve migration origin nested_state->size for
> + * the call to KVM_SET_NESTED_STATE but wish that our next call
> + * to KVM_GET_NESTED_STATE will use the maximum size supported by
> + * the kernel we're running on.
> + */
> + env->nested_state->size = nested_state_len;
> +
> + return kvm_vcpu_ioctl(CPU(cpu), KVM_GET_NESTED_STATE, env->nested_state);
> +}
> +
> int kvm_arch_put_registers(CPUState *cpu, int level)
> {
> X86CPU *x86_cpu = X86_CPU(cpu);
> @@ -3132,6 +3175,11 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>
> assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>
> + ret = kvm_put_nested_state(x86_cpu);
> + if (ret < 0) {
> + return ret;
> + }
> +
> if (level >= KVM_PUT_RESET_STATE) {
> ret = kvm_put_msr_feature_control(x86_cpu);
> if (ret < 0) {
> @@ -3247,6 +3295,10 @@ int kvm_arch_get_registers(CPUState *cs)
> if (ret < 0) {
> goto out;
> }
> + ret = kvm_get_nested_state(cpu);
> + if (ret < 0) {
> + goto out;
> + }
> ret = 0;
> out:
> cpu_sync_bndcs_hflags(&cpu->env);
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 41460be54b..45dbae6054 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -246,6 +246,15 @@ static int cpu_pre_save(void *opaque)
> env->segs[R_SS].flags &= ~(env->segs[R_SS].flags & DESC_DPL_MASK);
> }
>
> +#ifdef CONFIG_KVM
> + /* Verify we have nested virtualization state from kernel if required */
> + if (is_nested_virt_enabled(env) && !env->nested_state) {
> + error_report("Guest enabled nested virtualization but kernel "
> + "do not support saving nested state");
> + return -EINVAL;
> + }
> +#endif
> +
> return 0;
> }
>
> @@ -909,6 +918,176 @@ static const VMStateDescription vmstate_tsc_khz = {
> }
> };
>
> +#ifdef CONFIG_KVM
> +static bool vmx_vmcs12_needed(void *opaque)
> +{
> + struct kvm_nested_state *nested_state = opaque;
> + return (nested_state->size > offsetof(struct kvm_nested_state,
> + vmx.data[0].vmcs12));
Do you prefer this compared to checking explicitly? i.e. by:
return (nested_state->vmx.vmcs12_pa != -1ull);
> +}
> +
> +static const VMStateDescription vmstate_vmx_vmcs12_state = {
> + .name = "cpu/kvm_nested_state/vmx/vmcs12",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = vmx_vmcs12_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8_ARRAY(vmx.data[0].vmcs12,
> + struct kvm_nested_state, 0x1000),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static bool vmx_shadow_vmcs12_needed(void *opaque)
> +{
> + struct kvm_nested_state *nested_state = opaque;
> + return (nested_state->size > offsetof(struct kvm_nested_state,
> + vmx.data[0].shadow_vmcs12));
Nice trick on how to determine if to send shadow_vmcs12 without requiring to check
if vmcs12 indeed have VMCS-shadowing enabled and a valid vmcs-link-ptr. :)
> +}
> +
> +static const VMStateDescription vmstate_vmx_shadow_vmcs12_state = {
> + .name = "cpu/kvm_nested_state/vmx/shadow_vmcs12",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = vmx_shadow_vmcs12_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8_ARRAY(vmx.data[0].shadow_vmcs12,
> + struct kvm_nested_state, 0x1000),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static bool vmx_nested_state_needed(void *opaque)
> +{
> + struct kvm_nested_state *nested_state = opaque;
> +
> + return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
> + (nested_state->vmx.vmxon_pa != -1ull));
> +}
> +
> +static const VMStateDescription vmstate_vmx_nested_state = {
> + .name = "cpu/kvm_nested_state/vmx",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = vmx_nested_state_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_U64(vmx.vmxon_pa, struct kvm_nested_state),
> + VMSTATE_U64(vmx.vmcs_pa, struct kvm_nested_state),
> + VMSTATE_U16(vmx.smm.flags, struct kvm_nested_state),
> + VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription*[]) {
> + &vmstate_vmx_vmcs12_state,
> + &vmstate_vmx_shadow_vmcs12_state,
> + NULL,
> + }
> +};
> +
> +static bool svm_nested_state_needed(void *opaque)
> +{
> + struct kvm_nested_state *nested_state = opaque;
> +
> + return (nested_state->format == KVM_STATE_NESTED_FORMAT_SVM);
> +}
> +
> +static const VMStateDescription vmstate_svm_nested_state = {
> + .name = "cpu/kvm_nested_state/svm",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = svm_nested_state_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static bool nested_state_needed(void *opaque)
> +{
> + X86CPU *cpu = opaque;
> + CPUX86State *env = &cpu->env;
> +
> + return (is_vmx_enabled(env) && vmx_nested_state_needed(env->nested_state)) ||
> + (is_svm_enabled(env) && svm_nested_state_needed(env->nested_state));
> +}
As I specified in an earlier email in this patch-series, this is not entirely accurate.
In case vCPU is running L2 and entered SMM, then is_vmx_enabled() will return false because CR4 is set to 0 on entering SMM.
I consider deeming nested_state needed in case hflags specifies guest is in SMM mode. Any objection?
> +
> +static int nested_state_post_load(void *opaque, int version_id)
> +{
> + X86CPU *cpu = opaque;
> + CPUX86State *env = &cpu->env;
> + struct kvm_nested_state *nested_state = env->nested_state;
> + uint32_t min_nested_state_len = offsetof(struct kvm_nested_state, data);
> + uint32_t max_nested_state_len = kvm_max_nested_state_length();
> +
> + /*
> + * If our kernel don't support setting nested state
> + * and we have received nested state from migration stream,
> + * we need to fail migration
> + */
> + if (max_nested_state_len == 0) {
> + error_report("Received nested state when kernel cannot restore it");
> + return -EINVAL;
> + }
> +
> + /*
> + * Verify that the size of received nested_state struct
> + * at least cover required header and is not larger
> + * than the max size that our kernel support
> + */
> + if (nested_state->size < min_nested_state_len) {
> + error_report("Received nested state size less than min: "
> + "len=%d, min=%d",
> + nested_state->size, min_nested_state_len);
> + return -EINVAL;
> + }
> + if (nested_state->size > max_nested_state_len) {
> + error_report("Recieved unsupported nested state size: "
> + "nested_state->size=%d, max=%d",
> + nested_state->size, max_nested_state_len);
> + return -EINVAL;
> + }
> +
> + /* Verify format is valid */
> + if ((nested_state->format != KVM_STATE_NESTED_FORMAT_VMX) &&
> + (nested_state->format != KVM_STATE_NESTED_FORMAT_SVM)) {
> + error_report("Received invalid nested state format: %d",
> + nested_state->format);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_kvm_nested_state = {
> + .name = "cpu/kvm_nested_state",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_U16(flags, struct kvm_nested_state),
> + VMSTATE_U16(format, struct kvm_nested_state),
> + VMSTATE_U32(size, struct kvm_nested_state),
> + VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription*[]) {
> + &vmstate_vmx_nested_state,
> + &vmstate_svm_nested_state,
> + NULL
> + }
> +};
> +
> +static const VMStateDescription vmstate_nested_state = {
> + .name = "cpu/nested_state",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = nested_state_needed,
> + .post_load = nested_state_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT_POINTER(env.nested_state, X86CPU,
> + vmstate_kvm_nested_state,
> + struct kvm_nested_state),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +#endif
> +
> static bool mcg_ext_ctl_needed(void *opaque)
> {
> X86CPU *cpu = opaque;
> @@ -1148,6 +1327,9 @@ VMStateDescription vmstate_x86_cpu = {
> &vmstate_msr_intel_pt,
> &vmstate_msr_virt_ssbd,
> &vmstate_svm_npt,
> +#ifdef CONFIG_KVM
> + &vmstate_nested_state,
> +#endif
> NULL
> }
> };
> --
> 2.21.0
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] KVM: i386: Add support for save and restore nested state
2019-06-15 1:14 ` Liran Alon
@ 2019-06-17 17:31 ` Paolo Bonzini
0 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-17 17:31 UTC (permalink / raw)
To: Liran Alon; +Cc: qemu-devel, nikita.leshchenko
On 15/06/19 03:14, Liran Alon wrote:
>> @@ -1368,6 +1369,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> if (has_xsave) {
>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>> }
>> +
>> + nested_state_len = kvm_max_nested_state_length();
>> + if (nested_state_len > 0) {
>> + assert(nested_state_len >= offsetof(struct kvm_nested_state, data));
>> + env->nested_state = g_malloc0(nested_state_len);
>
> Paolo, why have you removed setting “env->nested_state->size = max_nested_state_len;”?
Because I confused the "nested_state_len == 0" check in
kvm_put_nested_state with "env->nested_state->size == 0".
> In addition, in my next patch-series I also added the following code here which is required:
>
> + if (IS_INTEL_CPU(env)) {
> + struct kvm_vmx_nested_state_hdr *vmx_hdr =
> + &env->nested_state->hdr.vmx_hdr;
> +
> + vmx_hdr->vmxon_pa = -1ull;
> + vmx_hdr->vmcs12_pa = -1ull;
> + }
Looks good.
>> + }
>> +
>> cpu->kvm_msr_buf = g_malloc0(MSR_BUF_SIZE);
>
> Note: In my next patch-series I have also added a new kvm_arch_destroy_vcpu() method which is called from kvm_destroy_vcpu().
> Similar to how kvm_arch_init_vcpu() is called from kvm_init_vcpu().
> I use it to free both cpu->kvm_msr_buf and env->nested_state.
Looks good too.
>>
>> if (!(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
>> @@ -3125,6 +3133,41 @@ static int kvm_get_debugregs(X86CPU *cpu)
>> return 0;
>> }
>>
>> +static int kvm_put_nested_state(X86CPU *cpu)
>> +{
>> + CPUX86State *env = &cpu->env;
>> + uint32_t nested_state_len = kvm_max_nested_state_length();
>> +
>> + if (nested_state_len == 0) {
>> + return 0;
>> + }
>> +
>> + assert(env->nested_state->size <= nested_state_len);
>> + return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_NESTED_STATE, env->nested_state);
>> +}
>> +
>> +static int kvm_get_nested_state(X86CPU *cpu)
>> +{
>> + CPUX86State *env = &cpu->env;
>> + uint32_t nested_state_len = kvm_max_nested_state_length();
>> +
>> + if (nested_state_len == 0) {
>> + return 0;
>> + }
>> +
>> + /*
>> + * It is possible that migration restored a smaller size into
>> + * nested_state->size than what our kernel supports.
>> + * We preserve migration origin nested_state->size for
>> + * the call to KVM_SET_NESTED_STATE but wish that our next call
>> + * to KVM_GET_NESTED_STATE will use the maximum size supported by
>> + * the kernel we're running on.
>> + */
>> + env->nested_state->size = nested_state_len;
>> +
>> + return kvm_vcpu_ioctl(CPU(cpu), KVM_GET_NESTED_STATE, env->nested_state);
>> +}
>> +
>> int kvm_arch_put_registers(CPUState *cpu, int level)
>> {
>> X86CPU *x86_cpu = X86_CPU(cpu);
>> @@ -3132,6 +3175,11 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>>
>> assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>>
>> + ret = kvm_put_nested_state(x86_cpu);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> if (level >= KVM_PUT_RESET_STATE) {
>> ret = kvm_put_msr_feature_control(x86_cpu);
>> if (ret < 0) {
>> @@ -3247,6 +3295,10 @@ int kvm_arch_get_registers(CPUState *cs)
>> if (ret < 0) {
>> goto out;
>> }
>> + ret = kvm_get_nested_state(cpu);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> ret = 0;
>> out:
>> cpu_sync_bndcs_hflags(&cpu->env);
>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>> index 41460be54b..45dbae6054 100644
>> --- a/target/i386/machine.c
>> +++ b/target/i386/machine.c
>> @@ -246,6 +246,15 @@ static int cpu_pre_save(void *opaque)
>> env->segs[R_SS].flags &= ~(env->segs[R_SS].flags & DESC_DPL_MASK);
>> }
>>
>> +#ifdef CONFIG_KVM
>> + /* Verify we have nested virtualization state from kernel if required */
>> + if (is_nested_virt_enabled(env) && !env->nested_state) {
>> + error_report("Guest enabled nested virtualization but kernel "
>> + "do not support saving nested state");
>> + return -EINVAL;
>> + }
>> +#endif
>> +
>> return 0;
>> }
>>
>> @@ -909,6 +918,176 @@ static const VMStateDescription vmstate_tsc_khz = {
>> }
>> };
>>
>> +#ifdef CONFIG_KVM
>> +static bool vmx_vmcs12_needed(void *opaque)
>> +{
>> + struct kvm_nested_state *nested_state = opaque;
>> + return (nested_state->size > offsetof(struct kvm_nested_state,
>> + vmx.data[0].vmcs12));
>
> Do you prefer this compared to checking explicitly? i.e. by:
> return (nested_state->vmx.vmcs12_pa != -1ull);
I think I do, it guarantees that we don't serialize gibberish from
vmx.data[0] and it's consistent with the vmx_shadow_vmcs12_needed check.
>> +static bool nested_state_needed(void *opaque)
>> +{
>> + X86CPU *cpu = opaque;
>> + CPUX86State *env = &cpu->env;
>> +
>> + return (is_vmx_enabled(env) && vmx_nested_state_needed(env->nested_state)) ||
>> + (is_svm_enabled(env) && svm_nested_state_needed(env->nested_state));
>> +}
>
> As I specified in an earlier email in this patch-series, this is not entirely accurate.
> In case vCPU is running L2 and entered SMM, then is_vmx_enabled() will return false because CR4 is set to 0 on entering SMM.
> I consider deeming nested_state needed in case hflags specifies guest is in SMM mode. Any objection?
See other answer, let's fix it in patch 7 instead.
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 7/7] Revert "target/i386: kvm: add VMX migration blocker"
2019-06-15 0:42 [Qemu-devel] [PATCH preliminary 0/7] target-i386/kvm: live migration support for nested VMX Paolo Bonzini
` (5 preceding siblings ...)
2019-06-15 0:42 ` [Qemu-devel] [PATCH 6/7] KVM: i386: Add support for save and restore nested state Paolo Bonzini
@ 2019-06-15 0:42 ` Paolo Bonzini
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2019-06-15 0:42 UTC (permalink / raw)
To: qemu-devel; +Cc: liran.alon, nikita.leshchenko
From: Liran Alon <liran.alon@oracle.com>
This reverts commit d98f26073bebddcd3da0ba1b86c3a34e840c0fb8.
The commit should be reverted because we now support nVMX migration.
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index d48fafa22b..0a01f40e73 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -943,7 +943,6 @@ static int hyperv_init_vcpu(X86CPU *cpu)
}
static Error *invtsc_mig_blocker;
-static Error *vmx_mig_blocker;
#define KVM_MAX_CPUID_ENTRIES 100
@@ -1308,17 +1307,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
!!(c->ecx & CPUID_EXT_SMX);
}
- if ((env->features[FEAT_1_ECX] & CPUID_EXT_VMX) && !vmx_mig_blocker) {
- error_setg(&vmx_mig_blocker,
- "Nested VMX virtualization does not support live migration yet");
- r = migrate_add_blocker(vmx_mig_blocker, &local_err);
- if (local_err) {
- error_report_err(local_err);
- error_free(vmx_mig_blocker);
- return r;
- }
- }
-
if (env->mcg_cap & MCG_LMCE_P) {
has_msr_mcg_ext_ctl = has_msr_feature_control = true;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread