* [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements
@ 2017-06-21 16:24 Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 01/23] hyperv: add header with protocol definitions Roman Kagan
` (22 more replies)
0 siblings, 23 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
This series applies miscellaneous fixes and enhancements to Hyper-V
emulation code in QEMU, and lays out the ground for VMBus devices.
v1 -> v2:
- drop the already merged patch
- split and rework SINTx and SVERSION msrs init
- factor out hyperv vcpu init to a function
- rework vp_index management
- distinguish kvm-only (== legacy) mode for SynIC
- use new capabilities recently submitted to KVM
- add compat logic for SynIC
- drop workaround for KVM zeroing SynIC pages
- minor fixes according to comments
Evgeny Yakovlev (1):
hyperv: set partition-wide MSRs only on first vcpu
Roman Kagan (22):
hyperv: add header with protocol definitions
update-linux-headers: prepare for hyperv.h removal
hyperv: ensure SINTx msrs are reset properly
hyperv: make SynIC version msr constant
[not to commit] add new hyperv-related caps
hyperv: ensure VP index equal to QEMU cpu_index
hyperv_testdev: refactor for readability
hyperv: cosmetic: g_malloc -> g_new
hyperv: synic: only setup ack notifier if there's a callback
hyperv: allow passing arbitrary data to sint ack callback
hyperv: address HvSintRoute by X86CPU pointer
hyperv: make HvSintRoute reference-counted
hyperv: qom-ify SynIC
hyperv: block SynIC use in QEMU in incompatible configurations
hyperv: make overlay pages for SynIC
hyperv: add synic message delivery
hyperv: add synic event flag signaling
hyperv: process SIGNAL_EVENT hypercall
hyperv: process POST_MESSAGE hypercall
hyperv_testdev: add SynIC message and event testmodes
MAINTAINERS: add myself and eyakovlev@ for hyperv*
hyperv: update copyright notices
include/hw/i386/pc.h | 5 +
linux-headers/linux/kvm.h | 2 +
target/i386/cpu.h | 16 +-
target/i386/hyperv.h | 40 ++-
target/i386/hyperv_proto.h | 257 ++++++++++++++++
hw/misc/hyperv_testdev.c | 267 +++++++++++++----
target/i386/cpu.c | 4 +-
target/i386/hyperv.c | 634 ++++++++++++++++++++++++++++++++++++++--
target/i386/kvm.c | 167 +++++++----
target/i386/machine.c | 24 +-
MAINTAINERS | 7 +
scripts/update-linux-headers.sh | 4 +-
12 files changed, 1256 insertions(+), 171 deletions(-)
create mode 100644 target/i386/hyperv_proto.h
--
2.9.4
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 01/23] hyperv: add header with protocol definitions
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 02/23] update-linux-headers: prepare for hyperv.h removal Roman Kagan
` (21 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
The definitions for Hyper-V emulation are currently taken from a header
imported from the Linux kernel.
However, as these describe a third-party protocol rather than a kernel
API, it probably wasn't a good idea to publish it in the kernel uapi.
This patch introduces a header that provides all the necessary
definitions, obsoleting the one coming from the kernel.
The new header supports (temporary) coexistence with the kernel one.
The constants explicitly named in the Hyper-V specification (e.g. msr
numbers) are defined in a non-conflicting way. Other constants and
types have got new names.
While at this, the protocol data structures are defined in a more
conventional way, without bitfields, enums, and excessive unions.
The code using this stuff is adjusted, too; it can now be built both
with and without the kernel header in the tree.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/cpu.h | 10 +-
target/i386/hyperv_proto.h | 257 +++++++++++++++++++++++++++++++++++++++++++++
target/i386/cpu.c | 4 +-
target/i386/hyperv.c | 6 +-
target/i386/kvm.c | 57 +++++-----
target/i386/machine.c | 15 ++-
6 files changed, 301 insertions(+), 48 deletions(-)
create mode 100644 target/i386/hyperv_proto.h
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index de0551f..464ed1e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -22,7 +22,7 @@
#include "qemu-common.h"
#include "cpu-qom.h"
-#include "standard-headers/asm-x86/hyperv.h"
+#include "hyperv_proto.h"
#ifdef TARGET_X86_64
#define TARGET_LONG_BITS 64
@@ -1093,15 +1093,15 @@ typedef struct CPUX86State {
uint64_t msr_hv_guest_os_id;
uint64_t msr_hv_vapic;
uint64_t msr_hv_tsc;
- uint64_t msr_hv_crash_params[HV_X64_MSR_CRASH_PARAMS];
+ uint64_t msr_hv_crash_params[HV_CRASH_PARAMS];
uint64_t msr_hv_runtime;
uint64_t msr_hv_synic_control;
uint64_t msr_hv_synic_version;
uint64_t msr_hv_synic_evt_page;
uint64_t msr_hv_synic_msg_page;
- uint64_t msr_hv_synic_sint[HV_SYNIC_SINT_COUNT];
- uint64_t msr_hv_stimer_config[HV_SYNIC_STIMER_COUNT];
- uint64_t msr_hv_stimer_count[HV_SYNIC_STIMER_COUNT];
+ uint64_t msr_hv_synic_sint[HV_SINT_COUNT];
+ uint64_t msr_hv_stimer_config[HV_STIMER_COUNT];
+ uint64_t msr_hv_stimer_count[HV_STIMER_COUNT];
/* exception/interrupt handling */
int error_code;
diff --git a/target/i386/hyperv_proto.h b/target/i386/hyperv_proto.h
new file mode 100644
index 0000000..1b3fa6e
--- /dev/null
+++ b/target/i386/hyperv_proto.h
@@ -0,0 +1,257 @@
+/*
+ * Definitions for Hyper-V guest/hypervisor interaction
+ *
+ * Copyright (C) 2017 Parallels International GmbH
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef TARGET_I386_HYPERV_PROTO_H
+#define TARGET_I386_HYPERV_PROTO_H
+
+#include "qemu/bitmap.h"
+
+#define HV_CPUID_VENDOR_AND_MAX_FUNCTIONS 0x40000000
+#define HV_CPUID_INTERFACE 0x40000001
+#define HV_CPUID_VERSION 0x40000002
+#define HV_CPUID_FEATURES 0x40000003
+#define HV_CPUID_ENLIGHTMENT_INFO 0x40000004
+#define HV_CPUID_IMPLEMENT_LIMITS 0x40000005
+#define HV_CPUID_MIN 0x40000005
+#define HV_CPUID_MAX 0x4000ffff
+#define HV_HYPERVISOR_PRESENT_BIT 0x80000000
+
+/*
+ * HV_CPUID_FEATURES.EAX bits
+ */
+#define HV_VP_RUNTIME_AVAILABLE (1u << 0)
+#define HV_TIME_REF_COUNT_AVAILABLE (1u << 1)
+#define HV_SYNIC_AVAILABLE (1u << 2)
+#define HV_SYNTIMERS_AVAILABLE (1u << 3)
+#define HV_APIC_ACCESS_AVAILABLE (1u << 4)
+#define HV_HYPERCALL_AVAILABLE (1u << 5)
+#define HV_VP_INDEX_AVAILABLE (1u << 6)
+#define HV_RESET_AVAILABLE (1u << 7)
+#define HV_REFERENCE_TSC_AVAILABLE (1u << 9)
+
+/*
+ * HV_CPUID_FEATURES.EDX bits
+ */
+#define HV_MWAIT_AVAILABLE (1u << 0)
+#define HV_GUEST_DEBUGGING_AVAILABLE (1u << 1)
+#define HV_PERF_MONITOR_AVAILABLE (1u << 2)
+#define HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE (1u << 3)
+#define HV_HYPERCALL_PARAMS_XMM_AVAILABLE (1u << 4)
+#define HV_GUEST_IDLE_STATE_AVAILABLE (1u << 5)
+#define HV_GUEST_CRASH_MSR_AVAILABLE (1u << 10)
+
+/*
+ * HV_CPUID_ENLIGHTMENT_INFO.EAX bits
+ */
+#define HV_AS_SWITCH_RECOMMENDED (1u << 0)
+#define HV_LOCAL_TLB_FLUSH_RECOMMENDED (1u << 1)
+#define HV_REMOTE_TLB_FLUSH_RECOMMENDED (1u << 2)
+#define HV_APIC_ACCESS_RECOMMENDED (1u << 3)
+#define HV_SYSTEM_RESET_RECOMMENDED (1u << 4)
+#define HV_RELAXED_TIMING_RECOMMENDED (1u << 5)
+
+/*
+ * Basic virtualized MSRs
+ */
+#define HV_X64_MSR_GUEST_OS_ID 0x40000000
+#define HV_X64_MSR_HYPERCALL 0x40000001
+#define HV_X64_MSR_VP_INDEX 0x40000002
+#define HV_X64_MSR_RESET 0x40000003
+#define HV_X64_MSR_VP_RUNTIME 0x40000010
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+#define HV_X64_MSR_REFERENCE_TSC 0x40000021
+#define HV_X64_MSR_TSC_FREQUENCY 0x40000022
+#define HV_X64_MSR_APIC_FREQUENCY 0x40000023
+
+/*
+ * Virtual APIC MSRs
+ */
+#define HV_X64_MSR_EOI 0x40000070
+#define HV_X64_MSR_ICR 0x40000071
+#define HV_X64_MSR_TPR 0x40000072
+#define HV_X64_MSR_APIC_ASSIST_PAGE 0x40000073
+
+/*
+ * Synthetic interrupt controller MSRs
+ */
+#define HV_X64_MSR_SCONTROL 0x40000080
+#define HV_X64_MSR_SVERSION 0x40000081
+#define HV_X64_MSR_SIEFP 0x40000082
+#define HV_X64_MSR_SIMP 0x40000083
+#define HV_X64_MSR_EOM 0x40000084
+#define HV_X64_MSR_SINT0 0x40000090
+#define HV_X64_MSR_SINT1 0x40000091
+#define HV_X64_MSR_SINT2 0x40000092
+#define HV_X64_MSR_SINT3 0x40000093
+#define HV_X64_MSR_SINT4 0x40000094
+#define HV_X64_MSR_SINT5 0x40000095
+#define HV_X64_MSR_SINT6 0x40000096
+#define HV_X64_MSR_SINT7 0x40000097
+#define HV_X64_MSR_SINT8 0x40000098
+#define HV_X64_MSR_SINT9 0x40000099
+#define HV_X64_MSR_SINT10 0x4000009A
+#define HV_X64_MSR_SINT11 0x4000009B
+#define HV_X64_MSR_SINT12 0x4000009C
+#define HV_X64_MSR_SINT13 0x4000009D
+#define HV_X64_MSR_SINT14 0x4000009E
+#define HV_X64_MSR_SINT15 0x4000009F
+
+/*
+ * Synthetic timer MSRs
+ */
+#define HV_X64_MSR_STIMER0_CONFIG 0x400000B0
+#define HV_X64_MSR_STIMER0_COUNT 0x400000B1
+#define HV_X64_MSR_STIMER1_CONFIG 0x400000B2
+#define HV_X64_MSR_STIMER1_COUNT 0x400000B3
+#define HV_X64_MSR_STIMER2_CONFIG 0x400000B4
+#define HV_X64_MSR_STIMER2_COUNT 0x400000B5
+#define HV_X64_MSR_STIMER3_CONFIG 0x400000B6
+#define HV_X64_MSR_STIMER3_COUNT 0x400000B7
+
+/*
+ * Guest crash notification MSRs
+ */
+#define HV_X64_MSR_CRASH_P0 0x40000100
+#define HV_X64_MSR_CRASH_P1 0x40000101
+#define HV_X64_MSR_CRASH_P2 0x40000102
+#define HV_X64_MSR_CRASH_P3 0x40000103
+#define HV_X64_MSR_CRASH_P4 0x40000104
+#define HV_CRASH_PARAMS (HV_X64_MSR_CRASH_P4 - HV_X64_MSR_CRASH_P0 + 1)
+#define HV_X64_MSR_CRASH_CTL 0x40000105
+#define HV_CRASH_CTL_NOTIFY (1ull << 63)
+
+/*
+ * Hypercall status code
+ */
+#define HV_STATUS_SUCCESS 0
+#define HV_STATUS_INVALID_HYPERCALL_CODE 2
+#define HV_STATUS_INVALID_HYPERCALL_INPUT 3
+#define HV_STATUS_INVALID_ALIGNMENT 4
+#define HV_STATUS_INVALID_PARAMETER 5
+#define HV_STATUS_INSUFFICIENT_MEMORY 11
+#define HV_STATUS_INVALID_CONNECTION_ID 18
+#define HV_STATUS_INSUFFICIENT_BUFFERS 19
+
+/*
+ * Hypercall numbers
+ */
+#define HV_POST_MESSAGE 0x005c
+#define HV_SIGNAL_EVENT 0x005d
+#define HV_HYPERCALL_FAST (1u << 16)
+
+/*
+ * Hypercall MSR bits
+ */
+#define HV_HYPERCALL_ENABLE (1u << 0)
+
+/*
+ * Synthetic interrupt controller definitions
+ */
+#define HV_SYNIC_VERSION 1
+#define HV_SINT_COUNT 16
+#define HV_SYNIC_ENABLE (1u << 0)
+#define HV_SIMP_ENABLE (1u << 0)
+#define HV_SIEFP_ENABLE (1u << 0)
+#define HV_SINT_MASKED (1u << 16)
+#define HV_SINT_AUTO_EOI (1u << 17)
+#define HV_SINT_VECTOR_MASK 0xff
+
+#define HV_STIMER_COUNT 4
+
+/*
+ * Message size
+ */
+#define HV_MESSAGE_PAYLOAD_SIZE 240
+
+/*
+ * Message types
+ */
+#define HV_MESSAGE_NONE 0x00000000
+#define HV_MESSAGE_VMBUS 0x00000001
+#define HV_MESSAGE_UNMAPPED_GPA 0x80000000
+#define HV_MESSAGE_GPA_INTERCEPT 0x80000001
+#define HV_MESSAGE_TIMER_EXPIRED 0x80000010
+#define HV_MESSAGE_INVALID_VP_REGISTER_VALUE 0x80000020
+#define HV_MESSAGE_UNRECOVERABLE_EXCEPTION 0x80000021
+#define HV_MESSAGE_UNSUPPORTED_FEATURE 0x80000022
+#define HV_MESSAGE_EVENTLOG_BUFFERCOMPLETE 0x80000040
+#define HV_MESSAGE_X64_IOPORT_INTERCEPT 0x80010000
+#define HV_MESSAGE_X64_MSR_INTERCEPT 0x80010001
+#define HV_MESSAGE_X64_CPUID_INTERCEPT 0x80010002
+#define HV_MESSAGE_X64_EXCEPTION_INTERCEPT 0x80010003
+#define HV_MESSAGE_X64_APIC_EOI 0x80010004
+#define HV_MESSAGE_X64_LEGACY_FP_ERROR 0x80010005
+
+/*
+ * Message flags
+ */
+#define HV_MESSAGE_FLAG_PENDING 0x1
+
+/*
+ * Event flags number per SINT
+ */
+#define HV_EVENT_FLAGS_COUNT (256 * 8)
+
+/*
+ * Connection id valid bits
+ */
+#define HV_CONNECTION_ID_MASK 0x00ffffff
+
+/*
+ * Input structure for POST_MESSAGE hypercall
+ */
+struct hyperv_post_message_input {
+ uint32_t connection_id;
+ uint32_t _reserved;
+ uint32_t message_type;
+ uint32_t payload_size;
+ uint8_t payload[HV_MESSAGE_PAYLOAD_SIZE];
+};
+
+/*
+ * Input structure for SIGNAL_EVENT hypercall
+ */
+struct hyperv_signal_event_input {
+ uint32_t connection_id;
+ uint16_t flag_number;
+ uint16_t _reserved_zero;
+};
+
+/*
+ * SynIC message structures
+ */
+struct hyperv_message_header {
+ uint32_t message_type;
+ uint8_t payload_size;
+ uint8_t message_flags; /* HV_MESSAGE_FLAG_XX */
+ uint8_t _reserved[2];
+ uint64_t sender;
+};
+
+struct hyperv_message {
+ struct hyperv_message_header header;
+ uint8_t payload[HV_MESSAGE_PAYLOAD_SIZE];
+};
+
+struct hyperv_message_page {
+ struct hyperv_message slot[HV_SINT_COUNT];
+};
+
+/*
+ * SynIC event flags structures
+ */
+struct hyperv_event_flags {
+ DECLARE_BITMAP(flags, HV_EVENT_FLAGS_COUNT);
+};
+
+struct hyperv_event_flags_page {
+ struct hyperv_event_flags slot[HV_SINT_COUNT];
+};
+
+#endif
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b2b1d20..631c14d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3812,12 +3812,12 @@ static GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
CPUX86State *env = &cpu->env;
GuestPanicInformation *panic_info = NULL;
- if (env->features[FEAT_HYPERV_EDX] & HV_X64_GUEST_CRASH_MSR_AVAILABLE) {
+ if (env->features[FEAT_HYPERV_EDX] & HV_GUEST_CRASH_MSR_AVAILABLE) {
panic_info = g_malloc0(sizeof(GuestPanicInformation));
panic_info->type = GUEST_PANIC_INFORMATION_TYPE_HYPER_V;
- assert(HV_X64_MSR_CRASH_PARAMS >= 5);
+ assert(HV_CRASH_PARAMS >= 5);
panic_info->u.hyper_v.arg1 = env->msr_hv_crash_params[0];
panic_info->u.hyper_v.arg2 = env->msr_hv_crash_params[1];
panic_info->u.hyper_v.arg3 = env->msr_hv_crash_params[2];
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 8545574..227185c 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -14,7 +14,7 @@
#include "qemu/osdep.h"
#include "qemu/main-loop.h"
#include "hyperv.h"
-#include "standard-headers/asm-x86/hyperv.h"
+#include "hyperv_proto.h"
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
{
@@ -50,8 +50,8 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
code = exit->u.hcall.input & 0xffff;
switch (code) {
- case HVCALL_POST_MESSAGE:
- case HVCALL_SIGNAL_EVENT:
+ case HV_POST_MESSAGE:
+ case HV_SIGNAL_EVENT:
default:
exit->u.hcall.result = HV_STATUS_INVALID_HYPERCALL_CODE;
return 0;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index ee36502..1c619dc 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -27,6 +27,7 @@
#include "sysemu/kvm_int.h"
#include "kvm_i386.h"
#include "hyperv.h"
+#include "hyperv_proto.h"
#include "exec/gdbstub.h"
#include "qemu/host-utils.h"
@@ -40,7 +41,6 @@
#include "hw/i386/x86-iommu.h"
#include "exec/ioport.h"
-#include "standard-headers/asm-x86/hyperv.h"
#include "hw/pci/pci.h"
#include "hw/pci/msi.h"
#include "hw/pci/msix.h"
@@ -622,29 +622,29 @@ static int hyperv_handle_properties(CPUState *cs)
}
if (cpu->hyperv_relaxed_timing) {
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_HYPERCALL_AVAILABLE;
}
if (cpu->hyperv_vapic) {
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_APIC_ACCESS_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_HYPERCALL_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_APIC_ACCESS_AVAILABLE;
}
if (cpu->hyperv_time) {
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_TIME_REF_COUNT_AVAILABLE;
- env->features[FEAT_HYPERV_EAX] |= 0x200;
+ env->features[FEAT_HYPERV_EAX] |= HV_HYPERCALL_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_TIME_REF_COUNT_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_REFERENCE_TSC_AVAILABLE;
}
if (cpu->hyperv_crash && has_msr_hv_crash) {
- env->features[FEAT_HYPERV_EDX] |= HV_X64_GUEST_CRASH_MSR_AVAILABLE;
+ env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE;
}
- env->features[FEAT_HYPERV_EDX] |= HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
+ env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
if (cpu->hyperv_reset && has_msr_hv_reset) {
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_RESET_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_RESET_AVAILABLE;
}
if (cpu->hyperv_vpindex && has_msr_hv_vpindex) {
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_INDEX_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_VP_INDEX_AVAILABLE;
}
if (cpu->hyperv_runtime && has_msr_hv_runtime) {
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_RUNTIME_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
}
if (cpu->hyperv_synic) {
int sint;
@@ -655,10 +655,10 @@ static int hyperv_handle_properties(CPUState *cs)
return -ENOSYS;
}
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_SYNIC_AVAILABLE;
- env->msr_hv_synic_version = HV_SYNIC_VERSION_1;
+ env->features[FEAT_HYPERV_EAX] |= HV_SYNIC_AVAILABLE;
+ env->msr_hv_synic_version = HV_SYNIC_VERSION;
for (sint = 0; sint < ARRAY_SIZE(env->msr_hv_synic_sint); sint++) {
- env->msr_hv_synic_sint[sint] = HV_SYNIC_SINT_MASKED;
+ env->msr_hv_synic_sint[sint] = HV_SINT_MASKED;
}
}
if (cpu->hyperv_stimer) {
@@ -666,7 +666,7 @@ static int hyperv_handle_properties(CPUState *cs)
fprintf(stderr, "Hyper-V timers aren't supported by kernel\n");
return -ENOSYS;
}
- env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_SYNTIMER_AVAILABLE;
+ env->features[FEAT_HYPERV_EAX] |= HV_SYNTIMERS_AVAILABLE;
}
return 0;
}
@@ -698,7 +698,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
/* Paravirtualization CPUIDs */
if (hyperv_enabled(cpu)) {
c = &cpuid_data.entries[cpuid_i++];
- c->function = HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
+ c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
if (!cpu->hyperv_vendor_id) {
memcpy(signature, "Microsoft Hv", 12);
} else {
@@ -711,13 +711,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
memset(signature, 0, 12);
memcpy(signature, cpu->hyperv_vendor_id, len);
}
- c->eax = HYPERV_CPUID_MIN;
+ c->eax = HV_CPUID_MIN;
c->ebx = signature[0];
c->ecx = signature[1];
c->edx = signature[2];
c = &cpuid_data.entries[cpuid_i++];
- c->function = HYPERV_CPUID_INTERFACE;
+ c->function = HV_CPUID_INTERFACE;
memcpy(signature, "Hv#1\0\0\0\0\0\0\0\0", 12);
c->eax = signature[0];
c->ebx = 0;
@@ -725,12 +725,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
c->edx = 0;
c = &cpuid_data.entries[cpuid_i++];
- c->function = HYPERV_CPUID_VERSION;
+ c->function = HV_CPUID_VERSION;
c->eax = 0x00001bbc;
c->ebx = 0x00060001;
c = &cpuid_data.entries[cpuid_i++];
- c->function = HYPERV_CPUID_FEATURES;
+ c->function = HV_CPUID_FEATURES;
r = hyperv_handle_properties(cs);
if (r) {
return r;
@@ -740,17 +740,17 @@ int kvm_arch_init_vcpu(CPUState *cs)
c->edx = env->features[FEAT_HYPERV_EDX];
c = &cpuid_data.entries[cpuid_i++];
- c->function = HYPERV_CPUID_ENLIGHTMENT_INFO;
+ c->function = HV_CPUID_ENLIGHTMENT_INFO;
if (cpu->hyperv_relaxed_timing) {
- c->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
+ c->eax |= HV_RELAXED_TIMING_RECOMMENDED;
}
if (cpu->hyperv_vapic) {
- c->eax |= HV_X64_APIC_ACCESS_RECOMMENDED;
+ c->eax |= HV_APIC_ACCESS_RECOMMENDED;
}
c->ebx = cpu->hyperv_spinlock_attempts;
c = &cpuid_data.entries[cpuid_i++];
- c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
+ c->function = HV_CPUID_IMPLEMENT_LIMITS;
c->eax = 0x40;
c->ebx = 0x40;
@@ -1734,12 +1734,11 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
if (has_msr_hv_crash) {
int j;
- for (j = 0; j < HV_X64_MSR_CRASH_PARAMS; j++)
+ for (j = 0; j < HV_CRASH_PARAMS; j++)
kvm_msr_entry_add(cpu, HV_X64_MSR_CRASH_P0 + j,
env->msr_hv_crash_params[j]);
- kvm_msr_entry_add(cpu, HV_X64_MSR_CRASH_CTL,
- HV_X64_MSR_CRASH_CTL_NOTIFY);
+ kvm_msr_entry_add(cpu, HV_X64_MSR_CRASH_CTL, HV_CRASH_CTL_NOTIFY);
}
if (has_msr_hv_runtime) {
kvm_msr_entry_add(cpu, HV_X64_MSR_VP_RUNTIME, env->msr_hv_runtime);
@@ -2144,7 +2143,7 @@ static int kvm_get_msrs(X86CPU *cpu)
if (has_msr_hv_crash) {
int j;
- for (j = 0; j < HV_X64_MSR_CRASH_PARAMS; j++) {
+ for (j = 0; j < HV_CRASH_PARAMS; j++) {
kvm_msr_entry_add(cpu, HV_X64_MSR_CRASH_P0 + j, 0);
}
}
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 8c7a822..ded5e34 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -566,7 +566,7 @@ static bool hyperv_crash_enable_needed(void *opaque)
CPUX86State *env = &cpu->env;
int i;
- for (i = 0; i < HV_X64_MSR_CRASH_PARAMS; i++) {
+ for (i = 0; i < HV_CRASH_PARAMS; i++) {
if (env->msr_hv_crash_params[i]) {
return true;
}
@@ -580,8 +580,7 @@ static const VMStateDescription vmstate_msr_hyperv_crash = {
.minimum_version_id = 1,
.needed = hyperv_crash_enable_needed,
.fields = (VMStateField[]) {
- VMSTATE_UINT64_ARRAY(env.msr_hv_crash_params,
- X86CPU, HV_X64_MSR_CRASH_PARAMS),
+ VMSTATE_UINT64_ARRAY(env.msr_hv_crash_params, X86CPU, HV_CRASH_PARAMS),
VMSTATE_END_OF_LIST()
}
};
@@ -639,8 +638,7 @@ static const VMStateDescription vmstate_msr_hyperv_synic = {
VMSTATE_UINT64(env.msr_hv_synic_control, X86CPU),
VMSTATE_UINT64(env.msr_hv_synic_evt_page, X86CPU),
VMSTATE_UINT64(env.msr_hv_synic_msg_page, X86CPU),
- VMSTATE_UINT64_ARRAY(env.msr_hv_synic_sint, X86CPU,
- HV_SYNIC_SINT_COUNT),
+ VMSTATE_UINT64_ARRAY(env.msr_hv_synic_sint, X86CPU, HV_SINT_COUNT),
VMSTATE_END_OF_LIST()
}
};
@@ -665,10 +663,9 @@ static const VMStateDescription vmstate_msr_hyperv_stimer = {
.minimum_version_id = 1,
.needed = hyperv_stimer_enable_needed,
.fields = (VMStateField[]) {
- VMSTATE_UINT64_ARRAY(env.msr_hv_stimer_config,
- X86CPU, HV_SYNIC_STIMER_COUNT),
- VMSTATE_UINT64_ARRAY(env.msr_hv_stimer_count,
- X86CPU, HV_SYNIC_STIMER_COUNT),
+ VMSTATE_UINT64_ARRAY(env.msr_hv_stimer_config, X86CPU,
+ HV_STIMER_COUNT),
+ VMSTATE_UINT64_ARRAY(env.msr_hv_stimer_count, X86CPU, HV_STIMER_COUNT),
VMSTATE_END_OF_LIST()
}
};
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 02/23] update-linux-headers: prepare for hyperv.h removal
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 01/23] hyperv: add header with protocol definitions Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 03/23] hyperv: set partition-wide MSRs only on first vcpu Roman Kagan
` (20 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
All definitions related to Hyper-V emulation are now taken from the QEMU
own header, so the one imported from the kernel is no longer needed.
Unfortunately it's included by kvm_para.h.
So, until this is fixed in the kernel, teach the header harvesting
script to substitute kernel's hyperv.h with a dummy.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
| 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index 2f906c4..ad80fe3 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -104,7 +104,9 @@ for arch in $ARCHLIST; do
cp "$tmpdir/include/asm/unistd-common.h" "$output/linux-headers/asm-arm/"
fi
if [ $arch = x86 ]; then
- cp_portable "$tmpdir/include/asm/hyperv.h" "$output/include/standard-headers/asm-x86/"
+ cat <<-EOF >"$output/include/standard-headers/asm-x86/hyperv.h"
+ /* this is a temporary placeholder until kvm_para.h stops including it */
+ EOF
cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-x86/"
cp "$tmpdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/"
cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/"
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 03/23] hyperv: set partition-wide MSRs only on first vcpu
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 01/23] hyperv: add header with protocol definitions Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 02/23] update-linux-headers: prepare for hyperv.h removal Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 04/23] hyperv: ensure SINTx msrs are reset properly Roman Kagan
` (19 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
From: Evgeny Yakovlev <eyakovlev@virtuozzo.com>
Hyper-V has a notion of partition-wide MSRs. Those MSRs are read and
written as usual on each VCPU, however the hypervisor maintains a single
global value for all VCPUs. Thus writing such an MSR from any single
VCPU affects the global value that is read by all other VCPUs.
This leads to an issue during VCPU hotplug: the zero-initialzied values
of those MSRs get synced into KVM and override the global values as has
already been set by the guest.
This change makes the partition-wide MSRs only be synchronized on the
first vcpu.
Signed-off-by: Evgeny Yakovlev <eyakovlev@virtuozzo.com>
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/cpu.h | 5 ++++-
target/i386/kvm.c | 20 ++++++++++++--------
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 464ed1e..ad8600d 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1089,10 +1089,13 @@ typedef struct CPUX86State {
uint64_t async_pf_en_msr;
uint64_t pv_eoi_en_msr;
+ /* Partition-wide HV MSRs, will be updated only on the first vcpu */
uint64_t msr_hv_hypercall;
uint64_t msr_hv_guest_os_id;
- uint64_t msr_hv_vapic;
uint64_t msr_hv_tsc;
+
+ /* Per-VCPU HV MSRs */
+ uint64_t msr_hv_vapic;
uint64_t msr_hv_crash_params[HV_CRASH_PARAMS];
uint64_t msr_hv_runtime;
uint64_t msr_hv_synic_control;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 1c619dc..183a85b 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1718,19 +1718,23 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
kvm_msr_entry_add(cpu, MSR_CORE_PERF_GLOBAL_CTRL,
env->msr_global_ctrl);
}
- if (has_msr_hv_hypercall) {
- kvm_msr_entry_add(cpu, HV_X64_MSR_GUEST_OS_ID,
- env->msr_hv_guest_os_id);
- kvm_msr_entry_add(cpu, HV_X64_MSR_HYPERCALL,
- env->msr_hv_hypercall);
+ /* Sync partition-wide MSRs only on first VCPU to avoid races */
+ if (current_cpu == first_cpu) {
+ if (has_msr_hv_hypercall) {
+ kvm_msr_entry_add(cpu, HV_X64_MSR_GUEST_OS_ID,
+ env->msr_hv_guest_os_id);
+ kvm_msr_entry_add(cpu, HV_X64_MSR_HYPERCALL,
+ env->msr_hv_hypercall);
+ }
+ if (cpu->hyperv_time) {
+ kvm_msr_entry_add(cpu, HV_X64_MSR_REFERENCE_TSC,
+ env->msr_hv_tsc);
+ }
}
if (cpu->hyperv_vapic) {
kvm_msr_entry_add(cpu, HV_X64_MSR_APIC_ASSIST_PAGE,
env->msr_hv_vapic);
}
- if (cpu->hyperv_time) {
- kvm_msr_entry_add(cpu, HV_X64_MSR_REFERENCE_TSC, env->msr_hv_tsc);
- }
if (has_msr_hv_crash) {
int j;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 04/23] hyperv: ensure SINTx msrs are reset properly
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (2 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 03/23] hyperv: set partition-wide MSRs only on first vcpu Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 05/23] hyperv: make SynIC version msr constant Roman Kagan
` (18 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Initially SINTx msrs should be in "masked" state. To ensure that
happens on *every* reset, move setting their values to
kvm_arch_vcpu_reset.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- split out of v1 patch 4
target/i386/kvm.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 183a85b..27404dd 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -647,8 +647,6 @@ static int hyperv_handle_properties(CPUState *cs)
env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
}
if (cpu->hyperv_synic) {
- int sint;
-
if (!has_msr_hv_synic ||
kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) {
fprintf(stderr, "Hyper-V SynIC is not supported by kernel\n");
@@ -657,9 +655,6 @@ static int hyperv_handle_properties(CPUState *cs)
env->features[FEAT_HYPERV_EAX] |= HV_SYNIC_AVAILABLE;
env->msr_hv_synic_version = HV_SYNIC_VERSION;
- for (sint = 0; sint < ARRAY_SIZE(env->msr_hv_synic_sint); sint++) {
- env->msr_hv_synic_sint[sint] = HV_SINT_MASKED;
- }
}
if (cpu->hyperv_stimer) {
if (!has_msr_hv_stimer) {
@@ -1039,6 +1034,13 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
} else {
env->mp_state = KVM_MP_STATE_RUNNABLE;
}
+
+ if (cpu->hyperv_synic) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) {
+ env->msr_hv_synic_sint[i] = HV_SINT_MASKED;
+ }
+ }
}
void kvm_arch_do_init_vcpu(X86CPU *cpu)
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 05/23] hyperv: make SynIC version msr constant
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (3 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 04/23] hyperv: ensure SINTx msrs are reset properly Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 06/23] [not to commit] add new hyperv-related caps Roman Kagan
` (17 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
The value of HV_X64_MSR_SVERSION is initialized once at vcpu init, and
is reset to zero on vcpu reset, which is wrong.
It is supposed to be a constant, so drop the field from X86CPU, set the
msr with the constant value, and don't bother getting it.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- split out of v1 patch 4
- make the value constant instead of keeping it on X86CPU
target/i386/cpu.h | 1 -
target/i386/kvm.c | 9 ++-------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ad8600d..917e3c4 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1099,7 +1099,6 @@ typedef struct CPUX86State {
uint64_t msr_hv_crash_params[HV_CRASH_PARAMS];
uint64_t msr_hv_runtime;
uint64_t msr_hv_synic_control;
- uint64_t msr_hv_synic_version;
uint64_t msr_hv_synic_evt_page;
uint64_t msr_hv_synic_msg_page;
uint64_t msr_hv_synic_sint[HV_SINT_COUNT];
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 27404dd..2795b63 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -654,7 +654,6 @@ static int hyperv_handle_properties(CPUState *cs)
}
env->features[FEAT_HYPERV_EAX] |= HV_SYNIC_AVAILABLE;
- env->msr_hv_synic_version = HV_SYNIC_VERSION;
}
if (cpu->hyperv_stimer) {
if (!has_msr_hv_stimer) {
@@ -1752,10 +1751,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
if (cpu->hyperv_synic) {
int j;
+ kvm_msr_entry_add(cpu, HV_X64_MSR_SVERSION, HV_SYNIC_VERSION);
+
kvm_msr_entry_add(cpu, HV_X64_MSR_SCONTROL,
env->msr_hv_synic_control);
- kvm_msr_entry_add(cpu, HV_X64_MSR_SVERSION,
- env->msr_hv_synic_version);
kvm_msr_entry_add(cpu, HV_X64_MSR_SIEFP,
env->msr_hv_synic_evt_page);
kvm_msr_entry_add(cpu, HV_X64_MSR_SIMP,
@@ -2160,7 +2159,6 @@ static int kvm_get_msrs(X86CPU *cpu)
uint32_t msr;
kvm_msr_entry_add(cpu, HV_X64_MSR_SCONTROL, 0);
- kvm_msr_entry_add(cpu, HV_X64_MSR_SVERSION, 0);
kvm_msr_entry_add(cpu, HV_X64_MSR_SIEFP, 0);
kvm_msr_entry_add(cpu, HV_X64_MSR_SIMP, 0);
for (msr = HV_X64_MSR_SINT0; msr <= HV_X64_MSR_SINT15; msr++) {
@@ -2364,9 +2362,6 @@ static int kvm_get_msrs(X86CPU *cpu)
case HV_X64_MSR_SCONTROL:
env->msr_hv_synic_control = msrs[i].data;
break;
- case HV_X64_MSR_SVERSION:
- env->msr_hv_synic_version = msrs[i].data;
- break;
case HV_X64_MSR_SIEFP:
env->msr_hv_synic_evt_page = msrs[i].data;
break;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 06/23] [not to commit] add new hyperv-related caps
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (4 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 05/23] hyperv: make SynIC version msr constant Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index Roman Kagan
` (16 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Add two hyperv-related caps
[they should arrive through the usual kernel header harvesting; this
patch allows to build QEMU before that happens].
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
| 2 ++
1 file changed, 2 insertions(+)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index d2892da..d7f532f 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -895,6 +895,8 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_SPAPR_TCE_VFIO 142
#define KVM_CAP_X86_GUEST_MWAIT 143
#define KVM_CAP_ARM_USER_IRQ 144
+#define KVM_CAP_HYPERV_SYNIC2 145
+#define KVM_CAP_HYPERV_VP_INDEX 146
#ifdef KVM_CAP_IRQ_ROUTING
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (5 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 06/23] [not to commit] add new hyperv-related caps Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-28 14:47 ` Igor Mammedov
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 08/23] hyperv_testdev: refactor for readability Roman Kagan
` (15 subsequent siblings)
22 siblings, 1 reply; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
spec as a sequential number which can't exceed the maximum number of
vCPUs per VM.
It has to be owned by QEMU in order to preserve it across migration.
However, the initial implementation in KVM didn't allow to set this
msr, and KVM used its own notion of VP index. Fortunately, the way
vCPUs are created in QEMU/KVM makes it likely that the KVM value is
equal to QEMU cpu_index.
So choose cpu_index as the value for vp_index, and push that to KVM on
kernels that support setting the msr. On older ones that don't, query
the kernel value and assert that it's in sync with QEMU.
Besides, since handling errors from vCPU init at hotplug time is
impossible, disable vCPU hotplug.
This patch also introduces accessor functions to wrap the mapping
between a vCPU and its vp_index. Besides, a few variables are renamed
to avoid confusion of vp_index with vcpu_id (== apic_id).
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- were patches 5, 6 in v1
- move vp_index initialization to hyperv_init_vcpu
- check capability before trying to set the msr
- set the msr on the usual kvm_put_msrs path
- disable cpu hotplug if msr is not settable
target/i386/hyperv.h | 5 ++++-
target/i386/hyperv.c | 16 +++++++++++++---
target/i386/kvm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 68 insertions(+), 4 deletions(-)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index 0c3b562..82f4757 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -32,11 +32,14 @@ struct HvSintRoute {
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
-HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
+HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
HvSintAckClb sint_ack_clb);
void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
+uint32_t hyperv_vp_index(X86CPU *cpu);
+X86CPU *hyperv_find_vcpu(uint32_t vp_index);
+
#endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 227185c..4f57447 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -16,6 +16,16 @@
#include "hyperv.h"
#include "hyperv_proto.h"
+uint32_t hyperv_vp_index(X86CPU *cpu)
+{
+ return CPU(cpu)->cpu_index;
+}
+
+X86CPU *hyperv_find_vcpu(uint32_t vp_index)
+{
+ return X86_CPU(qemu_get_cpu(vp_index));
+}
+
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
{
CPUX86State *env = &cpu->env;
@@ -72,7 +82,7 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
}
}
-HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
+HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
HvSintAckClb sint_ack_clb)
{
HvSintRoute *sint_route;
@@ -92,7 +102,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
event_notifier_set_handler(&sint_route->sint_ack_notifier,
kvm_hv_sint_ack_handler);
- gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vcpu_id, sint);
+ gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vp_index, sint);
if (gsi < 0) {
goto err_gsi;
}
@@ -105,7 +115,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
}
sint_route->gsi = gsi;
sint_route->sint_ack_clb = sint_ack_clb;
- sint_route->vcpu_id = vcpu_id;
+ sint_route->vcpu_id = vp_index;
sint_route->sint = sint;
return sint_route;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 2795b63..9bf7f08 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -86,6 +86,7 @@ static bool has_msr_hv_hypercall;
static bool has_msr_hv_crash;
static bool has_msr_hv_reset;
static bool has_msr_hv_vpindex;
+static bool is_hv_vpindex_settable;
static bool has_msr_hv_runtime;
static bool has_msr_hv_synic;
static bool has_msr_hv_stimer;
@@ -665,6 +666,43 @@ static int hyperv_handle_properties(CPUState *cs)
return 0;
}
+static int hyperv_init_vcpu(X86CPU *cpu)
+{
+ if (cpu->hyperv_vpindex && !is_hv_vpindex_settable) {
+ /*
+ * the kernel doesn't support setting vp_index; assert that its value
+ * is in sync
+ */
+ int ret;
+ struct {
+ struct kvm_msrs info;
+ struct kvm_msr_entry entries[1];
+ } msr_data = {
+ .info.nmsrs = 1,
+ .entries[0].index = HV_X64_MSR_VP_INDEX,
+ };
+
+ /*
+ * handling errors from vcpu init at hotplug time is impossible, so
+ * disallow cpu hotplug
+ */
+ MACHINE_GET_CLASS(current_machine)->hot_add_cpu = NULL;
+
+ ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
+ if (ret < 0) {
+ return ret;
+ }
+ assert(ret == 1);
+
+ if (msr_data.entries[0].data != hyperv_vp_index(cpu)) {
+ fprintf(stderr, "kernel's vp_index != QEMU's vp_index\n");
+ return -ENXIO;
+ }
+ }
+
+ return 0;
+}
+
static Error *invtsc_mig_blocker;
#define KVM_MAX_CPUID_ENTRIES 100
@@ -1013,6 +1051,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
has_msr_tsc_aux = false;
}
+ if (hyperv_enabled(cpu)) {
+ r = hyperv_init_vcpu(cpu);
+ if (r) {
+ goto fail;
+ }
+ }
+
return 0;
fail:
@@ -1204,6 +1249,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
has_pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
#endif
+ is_hv_vpindex_settable = kvm_check_extension(s, KVM_CAP_HYPERV_VP_INDEX);
+
ret = kvm_get_supported_msrs(s);
if (ret < 0) {
return ret;
@@ -1748,6 +1795,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
if (has_msr_hv_runtime) {
kvm_msr_entry_add(cpu, HV_X64_MSR_VP_RUNTIME, env->msr_hv_runtime);
}
+ if (cpu->hyperv_vpindex && has_msr_hv_vpindex &&
+ is_hv_vpindex_settable) {
+ kvm_msr_entry_add(cpu, HV_X64_MSR_VP_INDEX, hyperv_vp_index(cpu));
+ }
if (cpu->hyperv_synic) {
int j;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 08/23] hyperv_testdev: refactor for readability
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (6 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 09/23] hyperv: cosmetic: g_malloc -> g_new Roman Kagan
` (14 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Make hyperv_testdev slightly easier to follow and enhance in future.
For that, put the hyperv sint routes (wrapped in a helper structure) on
a linked list rather than a fixed-size array.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- was patch 7 in v1
- renamed the variables to distinguish vp_index from vcpu_id
hw/misc/hyperv_testdev.c | 114 ++++++++++++++++++++++-------------------------
1 file changed, 53 insertions(+), 61 deletions(-)
diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c
index dbd7cdd..b47af47 100644
--- a/hw/misc/hyperv_testdev.c
+++ b/hw/misc/hyperv_testdev.c
@@ -12,6 +12,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/queue.h"
#include <linux/kvm.h>
#include "hw/hw.h"
#include "hw/qdev.h"
@@ -20,12 +21,17 @@
#include "target/i386/hyperv.h"
#include "kvm_i386.h"
-#define HV_TEST_DEV_MAX_SINT_ROUTES 64
+typedef struct TestSintRoute {
+ QLIST_ENTRY(TestSintRoute) le;
+ uint8_t vpidx;
+ uint8_t sint;
+ HvSintRoute *sint_route;
+} TestSintRoute;
struct HypervTestDev {
ISADevice parent_obj;
MemoryRegion sint_control;
- HvSintRoute *sint_route[HV_TEST_DEV_MAX_SINT_ROUTES];
+ QLIST_HEAD(, TestSintRoute) sint_routes;
};
typedef struct HypervTestDev HypervTestDev;
@@ -39,88 +45,74 @@ enum {
HV_TEST_DEV_SINT_ROUTE_SET_SINT
};
-static int alloc_sint_route_index(HypervTestDev *dev)
+static void sint_route_create(HypervTestDev *dev, uint8_t vpidx, uint8_t sint)
{
- int i;
+ TestSintRoute *sint_route;
- for (i = 0; i < ARRAY_SIZE(dev->sint_route); i++) {
- if (dev->sint_route[i] == NULL) {
- return i;
- }
- }
- return -1;
-}
+ sint_route = g_new0(TestSintRoute, 1);
+ assert(sint_route);
-static void free_sint_route_index(HypervTestDev *dev, int i)
-{
- assert(i >= 0 && i < ARRAY_SIZE(dev->sint_route));
- dev->sint_route[i] = NULL;
+ sint_route->vpidx = vpidx;
+ sint_route->sint = sint;
+
+ sint_route->sint_route = kvm_hv_sint_route_create(vpidx, sint, NULL);
+ assert(sint_route->sint_route);
+
+ QLIST_INSERT_HEAD(&dev->sint_routes, sint_route, le);
}
-static int find_sint_route_index(HypervTestDev *dev, uint32_t vcpu_id,
- uint32_t sint)
+static TestSintRoute *sint_route_find(HypervTestDev *dev,
+ uint8_t vpidx, uint8_t sint)
{
- HvSintRoute *sint_route;
- int i;
+ TestSintRoute *sint_route;
- for (i = 0; i < ARRAY_SIZE(dev->sint_route); i++) {
- sint_route = dev->sint_route[i];
- if (sint_route && sint_route->vcpu_id == vcpu_id &&
- sint_route->sint == sint) {
- return i;
+ QLIST_FOREACH(sint_route, &dev->sint_routes, le) {
+ if (sint_route->vpidx == vpidx && sint_route->sint == sint) {
+ return sint_route;
}
}
- return -1;
+ assert(false);
+ return NULL;
}
-static void hv_synic_test_dev_control(HypervTestDev *dev, uint32_t ctl,
- uint32_t vcpu_id, uint32_t sint)
+static void sint_route_destroy(HypervTestDev *dev, uint8_t vpidx, uint8_t sint)
{
- int i;
- HvSintRoute *sint_route;
+ TestSintRoute *sint_route;
- switch (ctl) {
- case HV_TEST_DEV_SINT_ROUTE_CREATE:
- i = alloc_sint_route_index(dev);
- assert(i >= 0);
- sint_route = kvm_hv_sint_route_create(vcpu_id, sint, NULL);
- assert(sint_route);
- dev->sint_route[i] = sint_route;
- break;
- case HV_TEST_DEV_SINT_ROUTE_DESTROY:
- i = find_sint_route_index(dev, vcpu_id, sint);
- assert(i >= 0);
- sint_route = dev->sint_route[i];
- kvm_hv_sint_route_destroy(sint_route);
- free_sint_route_index(dev, i);
- break;
- case HV_TEST_DEV_SINT_ROUTE_SET_SINT:
- i = find_sint_route_index(dev, vcpu_id, sint);
- assert(i >= 0);
- sint_route = dev->sint_route[i];
- kvm_hv_sint_route_set_sint(sint_route);
- break;
- default:
- break;
- }
+ sint_route = sint_route_find(dev, vpidx, sint);
+ QLIST_REMOVE(sint_route, le);
+ kvm_hv_sint_route_destroy(sint_route->sint_route);
+ g_free(sint_route);
+}
+
+static void sint_route_set_sint(HypervTestDev *dev,
+ uint8_t vpidx, uint8_t sint)
+{
+ TestSintRoute *sint_route;
+
+ sint_route = sint_route_find(dev, vpidx, sint);
+
+ kvm_hv_sint_route_set_sint(sint_route->sint_route);
}
static void hv_test_dev_control(void *opaque, hwaddr addr, uint64_t data,
uint32_t len)
{
HypervTestDev *dev = HYPERV_TEST_DEV(opaque);
- uint8_t ctl;
+ uint8_t sint = data & 0xFF;
+ uint8_t vpidx = (data >> 8ULL) & 0xFF;
+ uint8_t ctl = (data >> 16ULL) & 0xFF;
- ctl = (data >> 16ULL) & 0xFF;
switch (ctl) {
case HV_TEST_DEV_SINT_ROUTE_CREATE:
+ sint_route_create(dev, vpidx, sint);
+ break;
case HV_TEST_DEV_SINT_ROUTE_DESTROY:
- case HV_TEST_DEV_SINT_ROUTE_SET_SINT: {
- uint8_t sint = data & 0xFF;
- uint8_t vcpu_id = (data >> 8ULL) & 0xFF;
- hv_synic_test_dev_control(dev, ctl, vcpu_id, sint);
+ sint_route_destroy(dev, vpidx, sint);
+ break;
+ case HV_TEST_DEV_SINT_ROUTE_SET_SINT:
+ sint_route_set_sint(dev, vpidx, sint);
break;
- }
default:
break;
}
@@ -139,7 +131,7 @@ static void hv_test_dev_realizefn(DeviceState *d, Error **errp)
HypervTestDev *dev = HYPERV_TEST_DEV(d);
MemoryRegion *io = isa_address_space_io(isa);
- memset(dev->sint_route, 0, sizeof(dev->sint_route));
+ QLIST_INIT(&dev->sint_routes);
memory_region_init_io(&dev->sint_control, OBJECT(dev),
&synic_test_sint_ops, dev,
"hyperv-testdev-ctl", 4);
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 09/23] hyperv: cosmetic: g_malloc -> g_new
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (7 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 08/23] hyperv_testdev: refactor for readability Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 10/23] hyperv: synic: only setup ack notifier if there's a callback Roman Kagan
` (13 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/hyperv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 4f57447..2121d8f 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -88,7 +88,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
HvSintRoute *sint_route;
int r, gsi;
- sint_route = g_malloc0(sizeof(*sint_route));
+ sint_route = g_new0(HvSintRoute, 1);
r = event_notifier_init(&sint_route->sint_set_notifier, false);
if (r) {
goto err;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 10/23] hyperv: synic: only setup ack notifier if there's a callback
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (8 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 09/23] hyperv: cosmetic: g_malloc -> g_new Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 11/23] hyperv: allow passing arbitrary data to sint ack callback Roman Kagan
` (12 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
There's no point setting up an sint ack notifier if no callback is
specified.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/hyperv.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 2121d8f..3594bd0 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -77,15 +77,14 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
HvSintRoute *sint_route = container_of(notifier, HvSintRoute,
sint_ack_notifier);
event_notifier_test_and_clear(notifier);
- if (sint_route->sint_ack_clb) {
- sint_route->sint_ack_clb(sint_route);
- }
+ sint_route->sint_ack_clb(sint_route);
}
HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
HvSintAckClb sint_ack_clb)
{
HvSintRoute *sint_route;
+ EventNotifier *ack_notifier;
int r, gsi;
sint_route = g_new0(HvSintRoute, 1);
@@ -94,13 +93,15 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
goto err;
}
- r = event_notifier_init(&sint_route->sint_ack_notifier, false);
- if (r) {
- goto err_sint_set_notifier;
- }
+ ack_notifier = sint_ack_clb ? &sint_route->sint_ack_notifier : NULL;
+ if (ack_notifier) {
+ r = event_notifier_init(ack_notifier, false);
+ if (r) {
+ goto err_sint_set_notifier;
+ }
- event_notifier_set_handler(&sint_route->sint_ack_notifier,
- kvm_hv_sint_ack_handler);
+ event_notifier_set_handler(ack_notifier, kvm_hv_sint_ack_handler);
+ }
gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vp_index, sint);
if (gsi < 0) {
@@ -109,7 +110,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
r = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
&sint_route->sint_set_notifier,
- &sint_route->sint_ack_notifier, gsi);
+ ack_notifier, gsi);
if (r) {
goto err_irqfd;
}
@@ -123,8 +124,10 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
err_irqfd:
kvm_irqchip_release_virq(kvm_state, gsi);
err_gsi:
- event_notifier_set_handler(&sint_route->sint_ack_notifier, NULL);
- event_notifier_cleanup(&sint_route->sint_ack_notifier);
+ if (ack_notifier) {
+ event_notifier_set_handler(ack_notifier, NULL);
+ event_notifier_cleanup(ack_notifier);
+ }
err_sint_set_notifier:
event_notifier_cleanup(&sint_route->sint_set_notifier);
err:
@@ -139,8 +142,10 @@ void kvm_hv_sint_route_destroy(HvSintRoute *sint_route)
&sint_route->sint_set_notifier,
sint_route->gsi);
kvm_irqchip_release_virq(kvm_state, sint_route->gsi);
- event_notifier_set_handler(&sint_route->sint_ack_notifier, NULL);
- event_notifier_cleanup(&sint_route->sint_ack_notifier);
+ if (sint_route->sint_ack_clb) {
+ event_notifier_set_handler(&sint_route->sint_ack_notifier, NULL);
+ event_notifier_cleanup(&sint_route->sint_ack_notifier);
+ }
event_notifier_cleanup(&sint_route->sint_set_notifier);
g_free(sint_route);
}
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 11/23] hyperv: allow passing arbitrary data to sint ack callback
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (9 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 10/23] hyperv: synic: only setup ack notifier if there's a callback Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 12/23] hyperv: address HvSintRoute by X86CPU pointer Roman Kagan
` (11 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Make sint ack callback accept an opaque pointer, that is stored on
sint_route at creation time.
This allows for more convenient interaction with the callback.
Besides, nothing outside hyperv.c should need to know the layout of
HvSintRoute fields any more so its declaration can be removed from the
header.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/hyperv.h | 14 +++-----------
hw/misc/hyperv_testdev.c | 2 +-
target/i386/hyperv.c | 16 ++++++++++++++--
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index 82f4757..93f7300 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -19,21 +19,13 @@
#include "qemu/event_notifier.h"
typedef struct HvSintRoute HvSintRoute;
-typedef void (*HvSintAckClb)(HvSintRoute *sint_route);
-
-struct HvSintRoute {
- uint32_t sint;
- uint32_t vcpu_id;
- int gsi;
- EventNotifier sint_set_notifier;
- EventNotifier sint_ack_notifier;
- HvSintAckClb sint_ack_clb;
-};
+typedef void (*HvSintAckClb)(void *data);
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
- HvSintAckClb sint_ack_clb);
+ HvSintAckClb sint_ack_clb,
+ void *sint_ack_clb_data);
void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c
index b47af47..827a8b1 100644
--- a/hw/misc/hyperv_testdev.c
+++ b/hw/misc/hyperv_testdev.c
@@ -55,7 +55,7 @@ static void sint_route_create(HypervTestDev *dev, uint8_t vpidx, uint8_t sint)
sint_route->vpidx = vpidx;
sint_route->sint = sint;
- sint_route->sint_route = kvm_hv_sint_route_create(vpidx, sint, NULL);
+ sint_route->sint_route = kvm_hv_sint_route_create(vpidx, sint, NULL, NULL);
assert(sint_route->sint_route);
QLIST_INSERT_HEAD(&dev->sint_routes, sint_route, le);
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 3594bd0..84ea228 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -16,6 +16,16 @@
#include "hyperv.h"
#include "hyperv_proto.h"
+struct HvSintRoute {
+ uint32_t sint;
+ uint32_t vcpu_id;
+ int gsi;
+ EventNotifier sint_set_notifier;
+ EventNotifier sint_ack_notifier;
+ HvSintAckClb sint_ack_clb;
+ void *sint_ack_clb_data;
+};
+
uint32_t hyperv_vp_index(X86CPU *cpu)
{
return CPU(cpu)->cpu_index;
@@ -77,11 +87,12 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
HvSintRoute *sint_route = container_of(notifier, HvSintRoute,
sint_ack_notifier);
event_notifier_test_and_clear(notifier);
- sint_route->sint_ack_clb(sint_route);
+ sint_route->sint_ack_clb(sint_route->sint_ack_clb_data);
}
HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
- HvSintAckClb sint_ack_clb)
+ HvSintAckClb sint_ack_clb,
+ void *sint_ack_clb_data)
{
HvSintRoute *sint_route;
EventNotifier *ack_notifier;
@@ -116,6 +127,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
}
sint_route->gsi = gsi;
sint_route->sint_ack_clb = sint_ack_clb;
+ sint_route->sint_ack_clb_data = sint_ack_clb_data;
sint_route->vcpu_id = vp_index;
sint_route->sint = sint;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 12/23] hyperv: address HvSintRoute by X86CPU pointer
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (10 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 11/23] hyperv: allow passing arbitrary data to sint ack callback Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 13/23] hyperv: make HvSintRoute reference-counted Roman Kagan
` (10 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Use X86CPU pointer to refer to the respective HvSintRoute instead of
vp_index. This is more convenient and also paves the way for future
enhancements.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- was patch 11 in v1
- pass vp_index to sint_route_create, and lookup X86CPU * inside
target/i386/hyperv.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 84ea228..b5831bf 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -18,7 +18,7 @@
struct HvSintRoute {
uint32_t sint;
- uint32_t vcpu_id;
+ X86CPU *cpu;
int gsi;
EventNotifier sint_set_notifier;
EventNotifier sint_ack_notifier;
@@ -97,6 +97,12 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
HvSintRoute *sint_route;
EventNotifier *ack_notifier;
int r, gsi;
+ X86CPU *cpu;
+
+ cpu = hyperv_find_vcpu(vp_index);
+ if (!cpu) {
+ return NULL;
+ }
sint_route = g_new0(HvSintRoute, 1);
r = event_notifier_init(&sint_route->sint_set_notifier, false);
@@ -128,7 +134,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
sint_route->gsi = gsi;
sint_route->sint_ack_clb = sint_ack_clb;
sint_route->sint_ack_clb_data = sint_ack_clb_data;
- sint_route->vcpu_id = vp_index;
+ sint_route->cpu = cpu;
sint_route->sint = sint;
return sint_route;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 13/23] hyperv: make HvSintRoute reference-counted
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (11 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 12/23] hyperv: address HvSintRoute by X86CPU pointer Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC Roman Kagan
` (9 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Multiple entities (e.g. VMBus devices) can use the same SINT route. To
make their lives easier in maintaining SINT route ownership, make it
reference-counted. Adjust the respective API names accordingly.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/hyperv.h | 10 +++++-----
hw/misc/hyperv_testdev.c | 4 ++--
target/i386/hyperv.c | 25 +++++++++++++++++++++----
3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index 93f7300..af5fc05 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -23,11 +23,11 @@ typedef void (*HvSintAckClb)(void *data);
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
-HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
- HvSintAckClb sint_ack_clb,
- void *sint_ack_clb_data);
-
-void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
+HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
+ HvSintAckClb sint_ack_clb,
+ void *sint_ack_clb_data);
+void hyperv_sint_route_ref(HvSintRoute *sint_route);
+void hyperv_sint_route_unref(HvSintRoute *sint_route);
int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c
index 827a8b1..fa435ab 100644
--- a/hw/misc/hyperv_testdev.c
+++ b/hw/misc/hyperv_testdev.c
@@ -55,7 +55,7 @@ static void sint_route_create(HypervTestDev *dev, uint8_t vpidx, uint8_t sint)
sint_route->vpidx = vpidx;
sint_route->sint = sint;
- sint_route->sint_route = kvm_hv_sint_route_create(vpidx, sint, NULL, NULL);
+ sint_route->sint_route = hyperv_sint_route_new(vpidx, sint, NULL, NULL);
assert(sint_route->sint_route);
QLIST_INSERT_HEAD(&dev->sint_routes, sint_route, le);
@@ -81,7 +81,7 @@ static void sint_route_destroy(HypervTestDev *dev, uint8_t vpidx, uint8_t sint)
sint_route = sint_route_find(dev, vpidx, sint);
QLIST_REMOVE(sint_route, le);
- kvm_hv_sint_route_destroy(sint_route->sint_route);
+ hyperv_sint_route_unref(sint_route->sint_route);
g_free(sint_route);
}
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index b5831bf..012c79d 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -24,6 +24,7 @@ struct HvSintRoute {
EventNotifier sint_ack_notifier;
HvSintAckClb sint_ack_clb;
void *sint_ack_clb_data;
+ unsigned refcount;
};
uint32_t hyperv_vp_index(X86CPU *cpu)
@@ -90,9 +91,9 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
sint_route->sint_ack_clb(sint_route->sint_ack_clb_data);
}
-HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
- HvSintAckClb sint_ack_clb,
- void *sint_ack_clb_data)
+HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
+ HvSintAckClb sint_ack_clb,
+ void *sint_ack_clb_data)
{
HvSintRoute *sint_route;
EventNotifier *ack_notifier;
@@ -136,6 +137,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
sint_route->sint_ack_clb_data = sint_ack_clb_data;
sint_route->cpu = cpu;
sint_route->sint = sint;
+ sint_route->refcount = 1;
return sint_route;
@@ -154,8 +156,23 @@ err:
return NULL;
}
-void kvm_hv_sint_route_destroy(HvSintRoute *sint_route)
+void hyperv_sint_route_ref(HvSintRoute *sint_route)
{
+ sint_route->refcount++;
+}
+
+void hyperv_sint_route_unref(HvSintRoute *sint_route)
+{
+ if (!sint_route) {
+ return;
+ }
+
+ assert(sint_route->refcount > 0);
+
+ if (--sint_route->refcount) {
+ return;
+ }
+
kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state,
&sint_route->sint_set_notifier,
sint_route->gsi);
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (12 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 13/23] hyperv: make HvSintRoute reference-counted Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-29 15:05 ` Igor Mammedov
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 15/23] hyperv: block SynIC use in QEMU in incompatible configurations Roman Kagan
` (8 subsequent siblings)
22 siblings, 1 reply; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Make Hyper-V SynIC a device which is attached as a child to X86CPU. For
now it only makes SynIC visibile in the qom hierarchy, and maintains its
internal fields in sync with the respecitve msrs of the parent cpu (the
fields will be used in followup patches).
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- was patch 13 in v1
- drop unnecessary QOM properties (but keep the fields)
- move KVM_CAP_HYPERV_SYNIC setting and SynIC creation to
hyperv_init_vcpu
target/i386/hyperv.h | 4 ++
target/i386/hyperv.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++-
target/i386/kvm.c | 14 ++++++-
target/i386/machine.c | 9 ++++
4 files changed, 134 insertions(+), 4 deletions(-)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index af5fc05..20bbd7b 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -34,4 +34,8 @@ int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
uint32_t hyperv_vp_index(X86CPU *cpu);
X86CPU *hyperv_find_vcpu(uint32_t vp_index);
+void hyperv_synic_add(X86CPU *cpu);
+void hyperv_synic_reset(X86CPU *cpu);
+void hyperv_synic_update(X86CPU *cpu);
+
#endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 012c79d..eff612c 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -13,12 +13,27 @@
#include "qemu/osdep.h"
#include "qemu/main-loop.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
#include "hyperv.h"
#include "hyperv_proto.h"
+typedef struct SynICState {
+ DeviceState parent_obj;
+
+ X86CPU *cpu;
+
+ bool enabled;
+ hwaddr msg_page_addr;
+ hwaddr evt_page_addr;
+} SynICState;
+
+#define TYPE_SYNIC "hyperv-synic"
+#define SYNIC(obj) OBJECT_CHECK(SynICState, (obj), TYPE_SYNIC)
+
struct HvSintRoute {
uint32_t sint;
- X86CPU *cpu;
+ SynICState *synic;
int gsi;
EventNotifier sint_set_notifier;
EventNotifier sint_ack_notifier;
@@ -37,6 +52,37 @@ X86CPU *hyperv_find_vcpu(uint32_t vp_index)
return X86_CPU(qemu_get_cpu(vp_index));
}
+static SynICState *get_synic(X86CPU *cpu)
+{
+ SynICState *synic =
+ SYNIC(object_resolve_path_component(OBJECT(cpu), "synic"));
+ assert(synic);
+ return synic;
+}
+
+static void synic_update_msg_page_addr(SynICState *synic)
+{
+ uint64_t msr = synic->cpu->env.msr_hv_synic_msg_page;
+ hwaddr new_addr = (msr & HV_SIMP_ENABLE) ? (msr & TARGET_PAGE_MASK) : 0;
+
+ synic->msg_page_addr = new_addr;
+}
+
+static void synic_update_evt_page_addr(SynICState *synic)
+{
+ uint64_t msr = synic->cpu->env.msr_hv_synic_evt_page;
+ hwaddr new_addr = (msr & HV_SIEFP_ENABLE) ? (msr & TARGET_PAGE_MASK) : 0;
+
+ synic->evt_page_addr = new_addr;
+}
+
+static void synic_update(SynICState *synic)
+{
+ synic->enabled = synic->cpu->env.msr_hv_synic_control & HV_SYNIC_ENABLE;
+ synic_update_msg_page_addr(synic);
+ synic_update_evt_page_addr(synic);
+}
+
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
{
CPUX86State *env = &cpu->env;
@@ -65,6 +111,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
default:
return -1;
}
+ synic_update(get_synic(cpu));
return 0;
case KVM_EXIT_HYPERV_HCALL: {
uint16_t code;
@@ -95,6 +142,7 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
HvSintAckClb sint_ack_clb,
void *sint_ack_clb_data)
{
+ SynICState *synic;
HvSintRoute *sint_route;
EventNotifier *ack_notifier;
int r, gsi;
@@ -105,6 +153,8 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
return NULL;
}
+ synic = get_synic(cpu);
+
sint_route = g_new0(HvSintRoute, 1);
r = event_notifier_init(&sint_route->sint_set_notifier, false);
if (r) {
@@ -135,7 +185,7 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
sint_route->gsi = gsi;
sint_route->sint_ack_clb = sint_ack_clb;
sint_route->sint_ack_clb_data = sint_ack_clb_data;
- sint_route->cpu = cpu;
+ sint_route->synic = synic;
sint_route->sint = sint;
sint_route->refcount = 1;
@@ -189,3 +239,60 @@ int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route)
{
return event_notifier_set(&sint_route->sint_set_notifier);
}
+
+static void synic_realize(DeviceState *dev, Error **errp)
+{
+ Object *obj = OBJECT(dev);
+ SynICState *synic = SYNIC(dev);
+
+ synic->cpu = X86_CPU(obj->parent);
+}
+
+static void synic_reset(DeviceState *dev)
+{
+ SynICState *synic = SYNIC(dev);
+ synic_update(synic);
+}
+
+static void synic_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = synic_realize;
+ dc->reset = synic_reset;
+ dc->user_creatable = false;
+}
+
+void hyperv_synic_add(X86CPU *cpu)
+{
+ Object *obj;
+
+ obj = object_new(TYPE_SYNIC);
+ object_property_add_child(OBJECT(cpu), "synic", obj, &error_abort);
+ object_unref(obj);
+ object_property_set_bool(obj, true, "realized", &error_abort);
+}
+
+void hyperv_synic_reset(X86CPU *cpu)
+{
+ device_reset(DEVICE(get_synic(cpu)));
+}
+
+void hyperv_synic_update(X86CPU *cpu)
+{
+ synic_update(get_synic(cpu));
+}
+
+static const TypeInfo synic_type_info = {
+ .name = TYPE_SYNIC,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(SynICState),
+ .class_init = synic_class_init,
+};
+
+static void synic_register_types(void)
+{
+ type_register_static(&synic_type_info);
+}
+
+type_init(synic_register_types)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 9bf7f08..eaa2df3 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -648,8 +648,7 @@ static int hyperv_handle_properties(CPUState *cs)
env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
}
if (cpu->hyperv_synic) {
- if (!has_msr_hv_synic ||
- kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) {
+ if (!has_msr_hv_synic) {
fprintf(stderr, "Hyper-V SynIC is not supported by kernel\n");
return -ENOSYS;
}
@@ -700,6 +699,15 @@ static int hyperv_init_vcpu(X86CPU *cpu)
}
}
+ if (cpu->hyperv_synic) {
+ if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
+ fprintf(stderr, "failed to enable Hyper-V SynIC\n");
+ return -ENOSYS;
+ }
+
+ hyperv_synic_add(cpu);
+ }
+
return 0;
}
@@ -1084,6 +1092,8 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) {
env->msr_hv_synic_sint[i] = HV_SINT_MASKED;
}
+
+ hyperv_synic_reset(cpu);
}
}
diff --git a/target/i386/machine.c b/target/i386/machine.c
index ded5e34..90cc3a9 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -7,6 +7,7 @@
#include "hw/i386/pc.h"
#include "hw/isa/isa.h"
#include "migration/cpu.h"
+#include "hyperv.h"
#include "sysemu/kvm.h"
@@ -629,11 +630,19 @@ static bool hyperv_synic_enable_needed(void *opaque)
return false;
}
+static int hyperv_synic_post_load(void *opaque, int version_id)
+{
+ X86CPU *cpu = opaque;
+ hyperv_synic_update(cpu);
+ return 0;
+}
+
static const VMStateDescription vmstate_msr_hyperv_synic = {
.name = "cpu/msr_hyperv_synic",
.version_id = 1,
.minimum_version_id = 1,
.needed = hyperv_synic_enable_needed,
+ .post_load = hyperv_synic_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINT64(env.msr_hv_synic_control, X86CPU),
VMSTATE_UINT64(env.msr_hv_synic_evt_page, X86CPU),
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 15/23] hyperv: block SynIC use in QEMU in incompatible configurations
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (13 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 16/23] hyperv: make overlay pages for SynIC Roman Kagan
` (7 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Certain configurations do not allow SynIC to be used in QEMU. In
particular,
- when hyperv_vpindex is off, SINT routes can't be used as they refer to
the destination vCPU by vp_index
- older KVM (which doesn't expose KVM_CAP_HYPERV_SYNIC2) zeroes out
SynIC message and event pages on every msr load, breaking migration
OTOH in-KVM users of SynIC -- SynIC timers -- do work in those
configurations, and we shouldn't stop the guest from using them.
Instead, introduce a SynIC property that disallows to use the SynIC
within QEMU but not in KVM. The property is set during vCPU init and
via compat logic (as older QEMU had no users for SynIC beyond the test
device).
Also a function is added that allows the devices to query the status of
SynIC support across vCPUs.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- new patch
include/hw/i386/pc.h | 5 +++++
target/i386/hyperv.h | 4 +++-
target/i386/hyperv.c | 39 ++++++++++++++++++++++++++++++++++++++-
target/i386/kvm.c | 12 ++++++++++--
4 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 233216a..72b5c62 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -389,6 +389,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
.property = "extended-tseg-mbytes",\
.value = stringify(0),\
},\
+ {\
+ .driver = "hyperv-synic",\
+ .property = "in-kvm-only",\
+ .value = "on",\
+ },\
#define PC_COMPAT_2_8 \
HW_COMPAT_2_8 \
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index 20bbd7b..7d8753e 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -34,8 +34,10 @@ int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
uint32_t hyperv_vp_index(X86CPU *cpu);
X86CPU *hyperv_find_vcpu(uint32_t vp_index);
-void hyperv_synic_add(X86CPU *cpu);
+void hyperv_synic_add(X86CPU *cpu, bool in_kvm_only);
void hyperv_synic_reset(X86CPU *cpu);
void hyperv_synic_update(X86CPU *cpu);
+bool hyperv_synic_usable(void);
+
#endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index eff612c..e183638 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -23,6 +23,8 @@ typedef struct SynICState {
X86CPU *cpu;
+ bool in_kvm_only;
+
bool enabled;
hwaddr msg_page_addr;
hwaddr evt_page_addr;
@@ -78,6 +80,10 @@ static void synic_update_evt_page_addr(SynICState *synic)
static void synic_update(SynICState *synic)
{
+ if (synic->in_kvm_only) {
+ return;
+ }
+
synic->enabled = synic->cpu->env.msr_hv_synic_control & HV_SYNIC_ENABLE;
synic_update_msg_page_addr(synic);
synic_update_evt_page_addr(synic);
@@ -154,6 +160,7 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
}
synic = get_synic(cpu);
+ assert(!synic->in_kvm_only);
sint_route = g_new0(HvSintRoute, 1);
r = event_notifier_init(&sint_route->sint_set_notifier, false);
@@ -240,6 +247,11 @@ int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route)
return event_notifier_set(&sint_route->sint_set_notifier);
}
+static Property synic_props[] = {
+ DEFINE_PROP_BOOL("in-kvm-only", SynICState, in_kvm_only, false),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void synic_realize(DeviceState *dev, Error **errp)
{
Object *obj = OBJECT(dev);
@@ -258,18 +270,24 @@ static void synic_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ dc->props = synic_props;
dc->realize = synic_realize;
dc->reset = synic_reset;
dc->user_creatable = false;
}
-void hyperv_synic_add(X86CPU *cpu)
+void hyperv_synic_add(X86CPU *cpu, bool in_kvm_only)
{
Object *obj;
+ SynICState *synic;
obj = object_new(TYPE_SYNIC);
object_property_add_child(OBJECT(cpu), "synic", obj, &error_abort);
object_unref(obj);
+
+ synic = SYNIC(obj);
+ synic->in_kvm_only = synic->in_kvm_only || in_kvm_only;
+
object_property_set_bool(obj, true, "realized", &error_abort);
}
@@ -283,6 +301,25 @@ void hyperv_synic_update(X86CPU *cpu)
synic_update(get_synic(cpu));
}
+bool hyperv_synic_usable(void)
+{
+ CPUState *cs;
+
+ CPU_FOREACH(cs) {
+ X86CPU *cpu = X86_CPU(cs);
+
+ if (!cpu->hyperv_synic) {
+ return false;
+ }
+
+ if (get_synic(cpu)->in_kvm_only) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
static const TypeInfo synic_type_info = {
.name = TYPE_SYNIC,
.parent = TYPE_DEVICE,
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index eaa2df3..8d1d232 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -700,12 +700,20 @@ static int hyperv_init_vcpu(X86CPU *cpu)
}
if (cpu->hyperv_synic) {
- if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
+ bool in_kvm_only = !cpu->hyperv_vpindex;
+
+ if (!in_kvm_only &&
+ kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC2, 0)) {
+ in_kvm_only = true;
+ }
+
+ if (in_kvm_only &&
+ kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
fprintf(stderr, "failed to enable Hyper-V SynIC\n");
return -ENOSYS;
}
- hyperv_synic_add(cpu);
+ hyperv_synic_add(cpu, in_kvm_only);
}
return 0;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 16/23] hyperv: make overlay pages for SynIC
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (14 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 15/23] hyperv: block SynIC use in QEMU in incompatible configurations Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 18/23] hyperv: add synic event flag signaling Roman Kagan
` (6 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Per Hyper-V spec, SynIC message and event flag pages are to be
implemented as so called overlay pages. That is, they are owned by the
hypervisor and, when mapped into the guest physical address space,
overlay the guest physical pages such that
1) the overlaid guest page becomes invisible to the guest CPUs until the
overlay page is turned off
2) the contents of the overlay page is preserved when it's turned off
and back on, even at a different address; it's only zeroed at vcpu
reset
This particular nature of SynIC message and event flag pages is ignored
in the current code, and guest physical pages are used directly instead.
This (mostly) works because the actual guests seem not to depend on the
features listed above.
This patch implements those pages as the spec mandates.
Since the extra RAM regions, which introduce migration incompatibility,
are only added when in_kvm_only == false, no extra compat logic is
necessary.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- was patch 15 in v1
- add comment on using async_safe_run_on_cpu
target/i386/hyperv.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 66 insertions(+), 6 deletions(-)
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index e183638..dca85de 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -15,6 +15,9 @@
#include "qemu/main-loop.h"
#include "qapi/error.h"
#include "hw/qdev-properties.h"
+#include "exec/address-spaces.h"
+#include "sysemu/cpus.h"
+#include "migration/vmstate.h"
#include "hyperv.h"
#include "hyperv_proto.h"
@@ -28,6 +31,10 @@ typedef struct SynICState {
bool enabled;
hwaddr msg_page_addr;
hwaddr evt_page_addr;
+ MemoryRegion msg_page_mr;
+ MemoryRegion evt_page_mr;
+ struct hyperv_message_page *msg_page;
+ struct hyperv_event_flags_page *evt_page;
} SynICState;
#define TYPE_SYNIC "hyperv-synic"
@@ -67,6 +74,17 @@ static void synic_update_msg_page_addr(SynICState *synic)
uint64_t msr = synic->cpu->env.msr_hv_synic_msg_page;
hwaddr new_addr = (msr & HV_SIMP_ENABLE) ? (msr & TARGET_PAGE_MASK) : 0;
+ if (new_addr == synic->msg_page_addr) {
+ return;
+ }
+
+ if (synic->msg_page_addr) {
+ memory_region_del_subregion(get_system_memory(), &synic->msg_page_mr);
+ }
+ if (new_addr) {
+ memory_region_add_subregion(get_system_memory(), new_addr,
+ &synic->msg_page_mr);
+ }
synic->msg_page_addr = new_addr;
}
@@ -75,6 +93,17 @@ static void synic_update_evt_page_addr(SynICState *synic)
uint64_t msr = synic->cpu->env.msr_hv_synic_evt_page;
hwaddr new_addr = (msr & HV_SIEFP_ENABLE) ? (msr & TARGET_PAGE_MASK) : 0;
+ if (new_addr == synic->evt_page_addr) {
+ return;
+ }
+
+ if (synic->evt_page_addr) {
+ memory_region_del_subregion(get_system_memory(), &synic->evt_page_mr);
+ }
+ if (new_addr) {
+ memory_region_add_subregion(get_system_memory(), new_addr,
+ &synic->evt_page_mr);
+ }
synic->evt_page_addr = new_addr;
}
@@ -89,6 +118,15 @@ static void synic_update(SynICState *synic)
synic_update_evt_page_addr(synic);
}
+
+static void async_synic_update(CPUState *cs, run_on_cpu_data data)
+{
+ SynICState *synic = data.host_ptr;
+ qemu_mutex_lock_iothread();
+ synic_update(synic);
+ qemu_mutex_unlock_iothread();
+}
+
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
{
CPUX86State *env = &cpu->env;
@@ -99,11 +137,6 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
return -1;
}
- /*
- * For now just track changes in SynIC control and msg/evt pages msr's.
- * When SynIC messaging/events processing will be added in future
- * here we will do messages queues flushing and pages remapping.
- */
switch (exit->u.synic.msr) {
case HV_X64_MSR_SCONTROL:
env->msr_hv_synic_control = exit->u.synic.control;
@@ -117,7 +150,13 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
default:
return -1;
}
- synic_update(get_synic(cpu));
+ /*
+ * this will run in this cpu thread before it returns to KVM, but in a
+ * safe environment (i.e. when all cpus are quiescent) -- this is
+ * necessary because we're changing memory hierarchy
+ */
+ async_safe_run_on_cpu(CPU(cpu), async_synic_update,
+ RUN_ON_CPU_HOST_PTR(get_synic(cpu)));
return 0;
case KVM_EXIT_HYPERV_HCALL: {
uint16_t code;
@@ -256,13 +295,34 @@ static void synic_realize(DeviceState *dev, Error **errp)
{
Object *obj = OBJECT(dev);
SynICState *synic = SYNIC(dev);
+ char *msgp_name, *evtp_name;
+ uint32_t vp_index;
synic->cpu = X86_CPU(obj->parent);
+
+ /* memory region names have to be globally unique */
+ vp_index = hyperv_vp_index(synic->cpu);
+ msgp_name = g_strdup_printf("synic-%u-msg-page", vp_index);
+ evtp_name = g_strdup_printf("synic-%u-evt-page", vp_index);
+
+ memory_region_init_ram(&synic->msg_page_mr, obj, msgp_name,
+ sizeof(*synic->msg_page), &error_abort);
+ memory_region_init_ram(&synic->evt_page_mr, obj, evtp_name,
+ sizeof(*synic->evt_page), &error_abort);
+ vmstate_register_ram(&synic->msg_page_mr, dev);
+ vmstate_register_ram(&synic->evt_page_mr, dev);
+ synic->msg_page = memory_region_get_ram_ptr(&synic->msg_page_mr);
+ synic->evt_page = memory_region_get_ram_ptr(&synic->evt_page_mr);
+
+ g_free(msgp_name);
+ g_free(evtp_name);
}
static void synic_reset(DeviceState *dev)
{
SynICState *synic = SYNIC(dev);
+ memset(synic->msg_page, 0, sizeof(*synic->msg_page));
+ memset(synic->evt_page, 0, sizeof(*synic->evt_page));
synic_update(synic);
}
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 18/23] hyperv: add synic event flag signaling
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (15 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 16/23] hyperv: make overlay pages for SynIC Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 19/23] hyperv: process SIGNAL_EVENT hypercall Roman Kagan
` (5 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Add infrastructure to signal SynIC event flags by atomically setting the
corresponding bit in the event flags page and firing a SINT if
necessary.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- swapped kvm_hv_sint_route_set_sint and memory_region_set_dirty
target/i386/hyperv.h | 2 ++
target/i386/hyperv.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index cc08ebb..f171f7b 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -41,4 +41,6 @@ bool hyperv_synic_usable(void);
int hyperv_post_msg(HvSintRoute *sint_route, struct hyperv_message *msg);
+int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno);
+
#endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 768dc2b..a96b03b 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -17,6 +17,7 @@
#include "hw/qdev-properties.h"
#include "exec/address-spaces.h"
#include "sysemu/cpus.h"
+#include "qemu/bitops.h"
#include "migration/vmstate.h"
#include "hyperv.h"
#include "hyperv_proto.h"
@@ -203,6 +204,37 @@ int hyperv_post_msg(HvSintRoute *sint_route, struct hyperv_message *src_msg)
return 0;
}
+/*
+ * Set given event flag for a given sint on a given vcpu, and signal the sint.
+ */
+int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno)
+{
+ int ret;
+ SynICState *synic = sint_route->synic;
+ unsigned long *flags, set_mask;
+ unsigned set_idx;
+
+ if (evtno > HV_EVENT_FLAGS_COUNT) {
+ return -EINVAL;
+ }
+ if (!synic->enabled || !synic->evt_page_addr) {
+ return -ENXIO;
+ }
+
+ set_idx = BIT_WORD(evtno);
+ set_mask = BIT_MASK(evtno);
+ flags = synic->evt_page->slot[sint_route->sint].flags;
+
+ if ((atomic_fetch_or(&flags[set_idx], set_mask) & set_mask) != set_mask) {
+ memory_region_set_dirty(&synic->evt_page_mr, 0,
+ sizeof(*synic->evt_page));
+ ret = kvm_hv_sint_route_set_sint(sint_route);
+ } else {
+ ret = 0;
+ }
+ return ret;
+}
+
static void async_synic_update(CPUState *cs, run_on_cpu_data data)
{
SynICState *synic = data.host_ptr;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 19/23] hyperv: process SIGNAL_EVENT hypercall
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (16 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 18/23] hyperv: add synic event flag signaling Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 20/23] hyperv: process POST_MESSAGE hypercall Roman Kagan
` (4 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Add handling of SIGNAL_EVENT hypercall. For that, provide an interface
to associate an EventNotifier with an event connection number, so that
it's signaled when the SIGNAL_EVENT hypercall with the matching
parameters is called by the guest.
TODO: we should be able to move this to KVM and avoid expensive user
exit just to look up an eventfd by connection number and signal it.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- use single mutex for evt and msg handler lists
target/i386/hyperv.h | 2 +
target/i386/hyperv.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 111 insertions(+), 4 deletions(-)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index f171f7b..46c6836 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -43,4 +43,6 @@ int hyperv_post_msg(HvSintRoute *sint_route, struct hyperv_message *msg);
int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno);
+int hyperv_set_evt_notifier(uint32_t conn_id, EventNotifier *notifier);
+
#endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index a96b03b..8abb418 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -18,6 +18,9 @@
#include "exec/address-spaces.h"
#include "sysemu/cpus.h"
#include "qemu/bitops.h"
+#include "qemu/queue.h"
+#include "qemu/rcu.h"
+#include "qemu/rcu_queue.h"
#include "migration/vmstate.h"
#include "hyperv.h"
#include "hyperv_proto.h"
@@ -243,6 +246,106 @@ static void async_synic_update(CPUState *cs, run_on_cpu_data data)
qemu_mutex_unlock_iothread();
}
+typedef struct EvtHandler {
+ struct rcu_head rcu;
+ QLIST_ENTRY(EvtHandler) le;
+ uint32_t conn_id;
+ EventNotifier *notifier;
+} EvtHandler;
+
+static QLIST_HEAD(, EvtHandler) evt_handlers;
+static QemuMutex handlers_mutex;
+
+static void __attribute__((constructor)) hv_init(void)
+{
+ QLIST_INIT(&evt_handlers);
+ qemu_mutex_init(&handlers_mutex);
+}
+
+int hyperv_set_evt_notifier(uint32_t conn_id, EventNotifier *notifier)
+{
+ int ret;
+ EvtHandler *eh;
+
+ qemu_mutex_lock(&handlers_mutex);
+ QLIST_FOREACH(eh, &evt_handlers, le) {
+ if (eh->conn_id == conn_id) {
+ if (notifier) {
+ ret = -EEXIST;
+ } else {
+ QLIST_REMOVE_RCU(eh, le);
+ g_free_rcu(eh, rcu);
+ ret = 0;
+ }
+ goto unlock;
+ }
+ }
+
+ if (notifier) {
+ eh = g_new(EvtHandler, 1);
+ eh->conn_id = conn_id;
+ eh->notifier = notifier;
+ QLIST_INSERT_HEAD_RCU(&evt_handlers, eh, le);
+ ret = 0;
+ } else {
+ ret = -ENOENT;
+ }
+unlock:
+ qemu_mutex_unlock(&handlers_mutex);
+ return ret;
+}
+
+static uint64_t sigevent_params(hwaddr addr, uint32_t *conn_id)
+{
+ uint64_t ret;
+ hwaddr len;
+ struct hyperv_signal_event_input *msg;
+
+ if (addr & (__alignof__(*msg) - 1)) {
+ return HV_STATUS_INVALID_ALIGNMENT;
+ }
+
+ len = sizeof(*msg);
+ msg = cpu_physical_memory_map(addr, &len, 0);
+ if (len < sizeof(*msg)) {
+ ret = HV_STATUS_INSUFFICIENT_MEMORY;
+ } else {
+ *conn_id = (msg->connection_id & HV_CONNECTION_ID_MASK) +
+ msg->flag_number;
+ ret = 0;
+ }
+ cpu_physical_memory_unmap(msg, len, 0, 0);
+ return ret;
+}
+
+static uint64_t hvcall_signal_event(uint64_t param, bool fast)
+{
+ uint64_t ret;
+ uint32_t conn_id;
+ EvtHandler *eh;
+
+ if (likely(fast)) {
+ conn_id = (param & 0xffffffff) + ((param >> 32) & 0xffff);
+ } else {
+ ret = sigevent_params(param, &conn_id);
+ if (ret) {
+ return ret;
+ }
+ }
+
+ ret = HV_STATUS_INVALID_CONNECTION_ID;
+ rcu_read_lock();
+ QLIST_FOREACH_RCU(eh, &evt_handlers, le) {
+ if (eh->conn_id == conn_id) {
+ event_notifier_set(eh->notifier);
+ ret = 0;
+ break;
+ }
+ }
+ rcu_read_unlock();
+ return ret;
+}
+
int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
{
CPUX86State *env = &cpu->env;
@@ -275,16 +378,18 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
RUN_ON_CPU_HOST_PTR(get_synic(cpu)));
return 0;
case KVM_EXIT_HYPERV_HCALL: {
- uint16_t code;
+ uint16_t code = exit->u.hcall.input & 0xffff;
+ bool fast = exit->u.hcall.input & HV_HYPERCALL_FAST;
+ uint64_t param = exit->u.hcall.params[0];
- code = exit->u.hcall.input & 0xffff;
switch (code) {
- case HV_POST_MESSAGE:
case HV_SIGNAL_EVENT:
+ exit->u.hcall.result = hvcall_signal_event(param, fast);
+ break;
default:
exit->u.hcall.result = HV_STATUS_INVALID_HYPERCALL_CODE;
- return 0;
}
+ return 0;
}
default:
return -1;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 20/23] hyperv: process POST_MESSAGE hypercall
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (17 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 19/23] hyperv: process SIGNAL_EVENT hypercall Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 21/23] hyperv_testdev: add SynIC message and event testmodes Roman Kagan
` (3 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Add handling of POST_MESSAGE hypercall. For that, add an interface to
regsiter a handler for the messages arrived from the guest on a
particular connection id (IOW set up a message connection in Hyper-V
speak).
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
- use single mutex for evt and msg handler lists
target/i386/hyperv.h | 5 +++
target/i386/hyperv.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 92 insertions(+)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index 46c6836..f101144 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -43,6 +43,11 @@ int hyperv_post_msg(HvSintRoute *sint_route, struct hyperv_message *msg);
int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno);
+struct hyperv_post_message_input;
+typedef uint64_t (*HvMsgHandler)(const struct hyperv_post_message_input *msg,
+ void *data);
+int hyperv_set_msg_handler(uint32_t conn_id, HvMsgHandler handler, void *data);
+
int hyperv_set_evt_notifier(uint32_t conn_id, EventNotifier *notifier);
#endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 8abb418..f779977 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -246,6 +246,14 @@ static void async_synic_update(CPUState *cs, run_on_cpu_data data)
qemu_mutex_unlock_iothread();
}
+typedef struct MsgHandler {
+ struct rcu_head rcu;
+ QLIST_ENTRY(MsgHandler) le;
+ uint32_t conn_id;
+ HvMsgHandler handler;
+ void *data;
+} MsgHandler;
+
typedef struct EvtHandler {
struct rcu_head rcu;
QLIST_ENTRY(EvtHandler) le;
@@ -253,15 +261,51 @@ typedef struct EvtHandler {
EventNotifier *notifier;
} EvtHandler;
+static QLIST_HEAD(, MsgHandler) msg_handlers;
static QLIST_HEAD(, EvtHandler) evt_handlers;
static QemuMutex handlers_mutex;
static void __attribute__((constructor)) hv_init(void)
{
+ QLIST_INIT(&msg_handlers);
QLIST_INIT(&evt_handlers);
qemu_mutex_init(&handlers_mutex);
}
+int hyperv_set_msg_handler(uint32_t conn_id, HvMsgHandler handler, void *data)
+{
+ int ret;
+ MsgHandler *mh;
+
+ qemu_mutex_lock(&handlers_mutex);
+ QLIST_FOREACH(mh, &msg_handlers, le) {
+ if (mh->conn_id == conn_id) {
+ if (handler) {
+ ret = -EEXIST;
+ } else {
+ QLIST_REMOVE_RCU(mh, le);
+ g_free_rcu(mh, rcu);
+ ret = 0;
+ }
+ goto unlock;
+ }
+ }
+
+ if (handler) {
+ mh = g_new(MsgHandler, 1);
+ mh->conn_id = conn_id;
+ mh->handler = handler;
+ mh->data = data;
+ QLIST_INSERT_HEAD_RCU(&msg_handlers, mh, le);
+ ret = 0;
+ } else {
+ ret = -ENOENT;
+ }
+unlock:
+ qemu_mutex_unlock(&handlers_mutex);
+ return ret;
+}
+
int hyperv_set_evt_notifier(uint32_t conn_id, EventNotifier *notifier)
{
int ret;
@@ -295,6 +339,46 @@ unlock:
return ret;
}
+static uint64_t hvcall_post_message(uint64_t param, bool fast)
+{
+ uint64_t ret;
+ hwaddr len;
+ struct hyperv_post_message_input *msg;
+ MsgHandler *mh;
+
+ if (fast) {
+ return HV_STATUS_INVALID_HYPERCALL_CODE;
+ }
+ if (param & (__alignof__(*msg) - 1)) {
+ return HV_STATUS_INVALID_ALIGNMENT;
+ }
+
+ len = sizeof(*msg);
+ msg = cpu_physical_memory_map(param, &len, 0);
+ if (len < sizeof(*msg)) {
+ ret = HV_STATUS_INSUFFICIENT_MEMORY;
+ goto unmap;
+ }
+ if (msg->payload_size > sizeof(msg->payload)) {
+ ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
+ goto unmap;
+ }
+
+ ret = HV_STATUS_INVALID_CONNECTION_ID;
+ rcu_read_lock();
+ QLIST_FOREACH_RCU(mh, &msg_handlers, le) {
+ if (mh->conn_id == (msg->connection_id & HV_CONNECTION_ID_MASK)) {
+ ret = mh->handler(msg, mh->data);
+ break;
+ }
+ }
+ rcu_read_unlock();
+
+unmap:
+ cpu_physical_memory_unmap(msg, len, 0, 0);
+ return ret;
+}
+
static uint64_t sigevent_params(hwaddr addr, uint32_t *conn_id)
{
uint64_t ret;
@@ -383,6 +467,9 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
uint64_t param = exit->u.hcall.params[0];
switch (code) {
+ case HV_POST_MESSAGE:
+ exit->u.hcall.result = hvcall_post_message(param, fast);
+ break;
case HV_SIGNAL_EVENT:
exit->u.hcall.result = hvcall_signal_event(param, fast);
break;
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 21/23] hyperv_testdev: add SynIC message and event testmodes
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (18 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 20/23] hyperv: process POST_MESSAGE hypercall Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 22/23] MAINTAINERS: add myself and eyakovlev@ for hyperv* Roman Kagan
` (2 subsequent siblings)
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Add testmodes for SynIC messages and events. The message or event
connection setup / teardown is initiated by the guest via new control
codes written to the test device port. Then the test connections bounce
the respective operations back to the guest, i.e. the incoming messages
are posted or the incoming events are signaled on the configured vCPUs.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
hw/misc/hyperv_testdev.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 161 insertions(+), 1 deletion(-)
diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c
index fa435ab..4fc7787 100644
--- a/hw/misc/hyperv_testdev.c
+++ b/hw/misc/hyperv_testdev.c
@@ -13,12 +13,15 @@
#include "qemu/osdep.h"
#include "qemu/queue.h"
+#include "qemu/event_notifier.h"
+#include "qemu/main-loop.h"
#include <linux/kvm.h>
#include "hw/hw.h"
#include "hw/qdev.h"
#include "hw/isa/isa.h"
#include "sysemu/kvm.h"
#include "target/i386/hyperv.h"
+#include "target/i386/hyperv_proto.h"
#include "kvm_i386.h"
typedef struct TestSintRoute {
@@ -28,10 +31,26 @@ typedef struct TestSintRoute {
HvSintRoute *sint_route;
} TestSintRoute;
+typedef struct TestMsgConn {
+ QLIST_ENTRY(TestMsgConn) le;
+ uint8_t conn_id;
+ HvSintRoute *sint_route;
+ struct hyperv_message msg;
+} TestMsgConn;
+
+typedef struct TestEvtConn {
+ QLIST_ENTRY(TestEvtConn) le;
+ uint8_t conn_id;
+ HvSintRoute *sint_route;
+ EventNotifier notifier;
+} TestEvtConn;
+
struct HypervTestDev {
ISADevice parent_obj;
MemoryRegion sint_control;
QLIST_HEAD(, TestSintRoute) sint_routes;
+ QLIST_HEAD(, TestMsgConn) msg_conns;
+ QLIST_HEAD(, TestEvtConn) evt_conns;
};
typedef struct HypervTestDev HypervTestDev;
@@ -42,7 +61,11 @@ typedef struct HypervTestDev HypervTestDev;
enum {
HV_TEST_DEV_SINT_ROUTE_CREATE = 1,
HV_TEST_DEV_SINT_ROUTE_DESTROY,
- HV_TEST_DEV_SINT_ROUTE_SET_SINT
+ HV_TEST_DEV_SINT_ROUTE_SET_SINT,
+ HV_TEST_DEV_MSG_CONN_CREATE,
+ HV_TEST_DEV_MSG_CONN_DESTROY,
+ HV_TEST_DEV_EVT_CONN_CREATE,
+ HV_TEST_DEV_EVT_CONN_DESTROY,
};
static void sint_route_create(HypervTestDev *dev, uint8_t vpidx, uint8_t sint)
@@ -95,6 +118,128 @@ static void sint_route_set_sint(HypervTestDev *dev,
kvm_hv_sint_route_set_sint(sint_route->sint_route);
}
+static void msg_cb(void *data, int status)
+{
+ TestMsgConn *conn = data;
+
+ assert(status == 0 || status == -EAGAIN);
+
+ if (!status) {
+ return;
+ }
+
+ /* no concurrent posting is expected so this should succeed */
+ assert(!hyperv_post_msg(conn->sint_route, &conn->msg));
+}
+
+static uint64_t msg_handler(const struct hyperv_post_message_input *msg,
+ void *data)
+{
+ int ret;
+ TestMsgConn *conn = data;
+
+ /* post the same message we've got */
+ conn->msg.header.message_type = msg->message_type;
+ assert(msg->payload_size < sizeof(conn->msg.payload));
+ conn->msg.header.payload_size = msg->payload_size;
+ memcpy(&conn->msg.payload, msg->payload, msg->payload_size);
+
+ ret = hyperv_post_msg(conn->sint_route, &conn->msg);
+
+ switch (ret) {
+ case 0:
+ return HV_STATUS_SUCCESS;
+ case -EAGAIN:
+ return HV_STATUS_INSUFFICIENT_BUFFERS;
+ default:
+ return HV_STATUS_INVALID_HYPERCALL_INPUT;
+ }
+}
+
+static void msg_conn_create(HypervTestDev *dev, uint8_t vpidx,
+ uint8_t sint, uint8_t conn_id)
+{
+ TestMsgConn *conn;
+
+ conn = g_new0(TestMsgConn, 1);
+ assert(conn);
+
+ conn->conn_id = conn_id;
+
+ conn->sint_route = hyperv_sint_route_new(vpidx, sint, msg_cb, conn);
+ assert(conn->sint_route);
+
+ assert(!hyperv_set_msg_handler(conn->conn_id, msg_handler, conn));
+
+ QLIST_INSERT_HEAD(&dev->msg_conns, conn, le);
+}
+
+static void msg_conn_destroy(HypervTestDev *dev, uint8_t conn_id)
+{
+ TestMsgConn *conn;
+
+ QLIST_FOREACH(conn, &dev->msg_conns, le) {
+ if (conn->conn_id == conn_id) {
+ QLIST_REMOVE(conn, le);
+ hyperv_set_msg_handler(conn->conn_id, NULL, NULL);
+ hyperv_sint_route_unref(conn->sint_route);
+ g_free(conn);
+ return;
+ }
+ }
+ assert(false);
+}
+
+static void evt_conn_handler(EventNotifier *notifier)
+{
+ TestEvtConn *conn = container_of(notifier, TestEvtConn, notifier);
+
+ event_notifier_test_and_clear(notifier);
+
+ /* signal the same event flag we've got */
+ assert(!hyperv_set_evt_flag(conn->sint_route, conn->conn_id));
+}
+
+static void evt_conn_create(HypervTestDev *dev, uint8_t vpidx,
+ uint8_t sint, uint8_t conn_id)
+{
+ TestEvtConn *conn;
+
+ conn = g_new0(TestEvtConn, 1);
+ assert(conn);
+
+ conn->conn_id = conn_id;
+
+ conn->sint_route = hyperv_sint_route_new(vpidx, sint, NULL, NULL);
+ assert(conn->sint_route);
+
+ assert(!event_notifier_init(&conn->notifier, false));
+
+ event_notifier_set_handler(&conn->notifier, evt_conn_handler);
+
+ assert(!hyperv_set_evt_notifier(conn_id, &conn->notifier));
+
+ QLIST_INSERT_HEAD(&dev->evt_conns, conn, le);
+}
+
+static void evt_conn_destroy(HypervTestDev *dev, uint8_t conn_id)
+{
+ TestEvtConn *conn;
+
+ QLIST_FOREACH(conn, &dev->evt_conns, le) {
+ if (conn->conn_id == conn_id) {
+ QLIST_REMOVE(conn, le);
+ hyperv_set_evt_notifier(conn->conn_id, NULL);
+ event_notifier_set_handler(&conn->notifier, NULL);
+ event_notifier_cleanup(&conn->notifier);
+ hyperv_sint_route_unref(conn->sint_route);
+ g_free(conn);
+ return;
+ }
+ }
+ assert(false);
+}
+
static void hv_test_dev_control(void *opaque, hwaddr addr, uint64_t data,
uint32_t len)
{
@@ -102,6 +247,7 @@ static void hv_test_dev_control(void *opaque, hwaddr addr, uint64_t data,
uint8_t sint = data & 0xFF;
uint8_t vpidx = (data >> 8ULL) & 0xFF;
uint8_t ctl = (data >> 16ULL) & 0xFF;
+ uint8_t conn_id = (data >> 24ULL) & 0xFF;
switch (ctl) {
case HV_TEST_DEV_SINT_ROUTE_CREATE:
@@ -113,6 +259,18 @@ static void hv_test_dev_control(void *opaque, hwaddr addr, uint64_t data,
case HV_TEST_DEV_SINT_ROUTE_SET_SINT:
sint_route_set_sint(dev, vpidx, sint);
break;
+ case HV_TEST_DEV_MSG_CONN_CREATE:
+ msg_conn_create(dev, vpidx, sint, conn_id);
+ break;
+ case HV_TEST_DEV_MSG_CONN_DESTROY:
+ msg_conn_destroy(dev, conn_id);
+ break;
+ case HV_TEST_DEV_EVT_CONN_CREATE:
+ evt_conn_create(dev, vpidx, sint, conn_id);
+ break;
+ case HV_TEST_DEV_EVT_CONN_DESTROY:
+ evt_conn_destroy(dev, conn_id);
+ break;
default:
break;
}
@@ -132,6 +290,8 @@ static void hv_test_dev_realizefn(DeviceState *d, Error **errp)
MemoryRegion *io = isa_address_space_io(isa);
QLIST_INIT(&dev->sint_routes);
+ QLIST_INIT(&dev->msg_conns);
+ QLIST_INIT(&dev->evt_conns);
memory_region_init_io(&dev->sint_control, OBJECT(dev),
&synic_test_sint_ops, dev,
"hyperv-testdev-ctl", 4);
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 22/23] MAINTAINERS: add myself and eyakovlev@ for hyperv*
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (19 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 21/23] hyperv_testdev: add SynIC message and event testmodes Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 23/23] hyperv: update copyright notices Roman Kagan
2017-06-29 15:20 ` [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Igor Mammedov
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 120788d..16c5422 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -852,6 +852,13 @@ F: hw/core/machine.c
F: hw/core/null-machine.c
F: include/hw/boards.h
+Hyper-V emulation core
+M: Roman Kagan <rkagan@virtuozzo.com>
+M: Evgeny Yakovlev <eyakovlev@virtuozzo.com>
+S: Supported
+F: target/i386/hyperv*
+F: hw/misc/hyperv*
+
Xtensa Machines
---------------
sim
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 23/23] hyperv: update copyright notices
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (20 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 22/23] MAINTAINERS: add myself and eyakovlev@ for hyperv* Roman Kagan
@ 2017-06-21 16:24 ` Roman Kagan
2017-06-29 15:20 ` [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Igor Mammedov
22 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-21 16:24 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Eduardo Habkost
Cc: Evgeny Yakovlev, Denis V . Lunev, Igor Mammedov
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
target/i386/hyperv.h | 1 +
hw/misc/hyperv_testdev.c | 1 +
target/i386/hyperv.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index f101144..cba6cbf 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -2,6 +2,7 @@
* QEMU KVM Hyper-V support
*
* Copyright (C) 2015 Andrey Smetanin <asmetanin@virtuozzo.com>
+ * Copyright (C) 2017 Parallels International GmbH
*
* Authors:
* Andrey Smetanin <asmetanin@virtuozzo.com>
diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c
index 4fc7787..57ee42d 100644
--- a/hw/misc/hyperv_testdev.c
+++ b/hw/misc/hyperv_testdev.c
@@ -2,6 +2,7 @@
* QEMU KVM Hyper-V test device to support Hyper-V kvm-unit-tests
*
* Copyright (C) 2015 Andrey Smetanin <asmetanin@virtuozzo.com>
+ * Copyright (C) 2017 Parallels International GmbH
*
* Authors:
* Andrey Smetanin <asmetanin@virtuozzo.com>
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index f779977..32c64af 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -2,6 +2,7 @@
* QEMU KVM Hyper-V support
*
* Copyright (C) 2015 Andrey Smetanin <asmetanin@virtuozzo.com>
+ * Copyright (C) 2017 Parallels International GmbH
*
* Authors:
* Andrey Smetanin <asmetanin@virtuozzo.com>
--
2.9.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index Roman Kagan
@ 2017-06-28 14:47 ` Igor Mammedov
2017-06-29 9:53 ` Roman Kagan
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2017-06-28 14:47 UTC (permalink / raw)
To: Roman Kagan
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Wed, 21 Jun 2017 19:24:08 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
> spec as a sequential number which can't exceed the maximum number of
> vCPUs per VM.
>
> It has to be owned by QEMU in order to preserve it across migration.
>
> However, the initial implementation in KVM didn't allow to set this
> msr, and KVM used its own notion of VP index. Fortunately, the way
> vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> equal to QEMU cpu_index.
>
> So choose cpu_index as the value for vp_index, and push that to KVM on
> kernels that support setting the msr. On older ones that don't, query
> the kernel value and assert that it's in sync with QEMU.
>
> Besides, since handling errors from vCPU init at hotplug time is
> impossible, disable vCPU hotplug.
proper place to check if cpu might be created is at
pc_cpu_pre_plug() where you can gracefully abort cpu creation process.
Also it's possible to create cold-plugged CPUs in out of order
sequence using
-device cpu-foo on CLI
will be hyperv kvm/guest side ok with it?
> This patch also introduces accessor functions to wrap the mapping
> between a vCPU and its vp_index. Besides, a few variables are renamed
> to avoid confusion of vp_index with vcpu_id (== apic_id).
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> v1 -> v2:
> - were patches 5, 6 in v1
> - move vp_index initialization to hyperv_init_vcpu
> - check capability before trying to set the msr
> - set the msr on the usual kvm_put_msrs path
> - disable cpu hotplug if msr is not settable
>
> target/i386/hyperv.h | 5 ++++-
> target/i386/hyperv.c | 16 +++++++++++++---
> target/i386/kvm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
> index 0c3b562..82f4757 100644
> --- a/target/i386/hyperv.h
> +++ b/target/i386/hyperv.h
> @@ -32,11 +32,14 @@ struct HvSintRoute {
>
> int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
>
> -HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> +HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
> HvSintAckClb sint_ack_clb);
>
> void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
>
> int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
>
> +uint32_t hyperv_vp_index(X86CPU *cpu);
> +X86CPU *hyperv_find_vcpu(uint32_t vp_index);
> +
> #endif
> diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
> index 227185c..4f57447 100644
> --- a/target/i386/hyperv.c
> +++ b/target/i386/hyperv.c
> @@ -16,6 +16,16 @@
> #include "hyperv.h"
> #include "hyperv_proto.h"
>
> +uint32_t hyperv_vp_index(X86CPU *cpu)
> +{
> + return CPU(cpu)->cpu_index;
> +}
> +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> +{
> + return X86_CPU(qemu_get_cpu(vp_index));
> +}
this helper isn't used in this patch, add it in the patch that would actually use it
also if qemu_get_cpu() were called from each CPU init,
it would incur O(N^2) complexity, could you do without it?
> +
> int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
> {
> CPUX86State *env = &cpu->env;
> @@ -72,7 +82,7 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
> }
> }
>
> -HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> +HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
> HvSintAckClb sint_ack_clb)
> {
> HvSintRoute *sint_route;
> @@ -92,7 +102,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> event_notifier_set_handler(&sint_route->sint_ack_notifier,
> kvm_hv_sint_ack_handler);
>
> - gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vcpu_id, sint);
> + gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vp_index, sint);
> if (gsi < 0) {
> goto err_gsi;
> }
> @@ -105,7 +115,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> }
> sint_route->gsi = gsi;
> sint_route->sint_ack_clb = sint_ack_clb;
> - sint_route->vcpu_id = vcpu_id;
> + sint_route->vcpu_id = vp_index;
^^^ - shouldn't it also be re-named?
maybe split all renaming into separate patch ...
> sint_route->sint = sint;
>
> return sint_route;
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 2795b63..9bf7f08 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -86,6 +86,7 @@ static bool has_msr_hv_hypercall;
> static bool has_msr_hv_crash;
> static bool has_msr_hv_reset;
> static bool has_msr_hv_vpindex;
> +static bool is_hv_vpindex_settable;
> static bool has_msr_hv_runtime;
> static bool has_msr_hv_synic;
> static bool has_msr_hv_stimer;
> @@ -665,6 +666,43 @@ static int hyperv_handle_properties(CPUState *cs)
> return 0;
> }
>
> +static int hyperv_init_vcpu(X86CPU *cpu)
> +{
> + if (cpu->hyperv_vpindex && !is_hv_vpindex_settable) {
> + /*
> + * the kernel doesn't support setting vp_index; assert that its value
> + * is in sync
> + */
> + int ret;
> + struct {
> + struct kvm_msrs info;
> + struct kvm_msr_entry entries[1];
> + } msr_data = {
> + .info.nmsrs = 1,
> + .entries[0].index = HV_X64_MSR_VP_INDEX,
> + };
> +
> + /*
> + * handling errors from vcpu init at hotplug time is impossible, so
> + * disallow cpu hotplug
> + */
> + MACHINE_GET_CLASS(current_machine)->hot_add_cpu = NULL;
one shouldn't alter machine this way nor
it would disable cpu hotplug, it would disable only cpu-add interface
but device_add() would still work.
> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> + if (ret < 0) {
when this can fail?
> + return ret;
> + }
> + assert(ret == 1);
> +
> + if (msr_data.entries[0].data != hyperv_vp_index(cpu)) {
> + fprintf(stderr, "kernel's vp_index != QEMU's vp_index\n");
error_report()
> + return -ENXIO;
> + }
> + }
> +
> + return 0;
> +}
> +
> static Error *invtsc_mig_blocker;
>
> #define KVM_MAX_CPUID_ENTRIES 100
> @@ -1013,6 +1051,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
> has_msr_tsc_aux = false;
> }
>
> + if (hyperv_enabled(cpu)) {
> + r = hyperv_init_vcpu(cpu);
> + if (r) {
> + goto fail;
> + }
> + }
> +
> return 0;
>
> fail:
> @@ -1204,6 +1249,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> has_pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
> #endif
>
> + is_hv_vpindex_settable = kvm_check_extension(s, KVM_CAP_HYPERV_VP_INDEX);
> +
> ret = kvm_get_supported_msrs(s);
> if (ret < 0) {
> return ret;
> @@ -1748,6 +1795,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> if (has_msr_hv_runtime) {
> kvm_msr_entry_add(cpu, HV_X64_MSR_VP_RUNTIME, env->msr_hv_runtime);
> }
> + if (cpu->hyperv_vpindex && has_msr_hv_vpindex &&
> + is_hv_vpindex_settable) {
> + kvm_msr_entry_add(cpu, HV_X64_MSR_VP_INDEX, hyperv_vp_index(cpu));
> + }
> if (cpu->hyperv_synic) {
> int j;
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-28 14:47 ` Igor Mammedov
@ 2017-06-29 9:53 ` Roman Kagan
2017-06-29 11:53 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Roman Kagan @ 2017-06-29 9:53 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Wed, Jun 28, 2017 at 04:47:43PM +0200, Igor Mammedov wrote:
> On Wed, 21 Jun 2017 19:24:08 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
>
> > Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> > queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
> > spec as a sequential number which can't exceed the maximum number of
> > vCPUs per VM.
> >
> > It has to be owned by QEMU in order to preserve it across migration.
> >
> > However, the initial implementation in KVM didn't allow to set this
> > msr, and KVM used its own notion of VP index. Fortunately, the way
> > vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> > equal to QEMU cpu_index.
> >
> > So choose cpu_index as the value for vp_index, and push that to KVM on
> > kernels that support setting the msr. On older ones that don't, query
> > the kernel value and assert that it's in sync with QEMU.
> >
> > Besides, since handling errors from vCPU init at hotplug time is
> > impossible, disable vCPU hotplug.
> proper place to check if cpu might be created is at
> pc_cpu_pre_plug() where you can gracefully abort cpu creation process.
Thanks for the suggestion, I'll rework it this way.
> Also it's possible to create cold-plugged CPUs in out of order
> sequence using
> -device cpu-foo on CLI
> will be hyperv kvm/guest side ok with it?
On kernels that support setting HV_X64_MSR_VP_INDEX QEMU will
synchronize all sides. On kernels that don't, if out-of-order creation
results in vp_index mismatch between the kernel and QEMU, vcpu creation
will fail.
> > This patch also introduces accessor functions to wrap the mapping
> > between a vCPU and its vp_index. Besides, a few variables are renamed
> > to avoid confusion of vp_index with vcpu_id (== apic_id).
> >
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > ---
> > v1 -> v2:
> > - were patches 5, 6 in v1
> > - move vp_index initialization to hyperv_init_vcpu
> > - check capability before trying to set the msr
> > - set the msr on the usual kvm_put_msrs path
> > - disable cpu hotplug if msr is not settable
> >
> > target/i386/hyperv.h | 5 ++++-
> > target/i386/hyperv.c | 16 +++++++++++++---
> > target/i386/kvm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 68 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
> > index 0c3b562..82f4757 100644
> > --- a/target/i386/hyperv.h
> > +++ b/target/i386/hyperv.h
> > @@ -32,11 +32,14 @@ struct HvSintRoute {
> >
> > int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
> >
> > -HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> > +HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
> > HvSintAckClb sint_ack_clb);
> >
> > void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
> >
> > int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
> >
> > +uint32_t hyperv_vp_index(X86CPU *cpu);
> > +X86CPU *hyperv_find_vcpu(uint32_t vp_index);
> > +
> > #endif
> > diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
> > index 227185c..4f57447 100644
> > --- a/target/i386/hyperv.c
> > +++ b/target/i386/hyperv.c
> > @@ -16,6 +16,16 @@
> > #include "hyperv.h"
> > #include "hyperv_proto.h"
> >
> > +uint32_t hyperv_vp_index(X86CPU *cpu)
> > +{
> > + return CPU(cpu)->cpu_index;
> > +}
>
>
> > +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> > +{
> > + return X86_CPU(qemu_get_cpu(vp_index));
> > +}
> this helper isn't used in this patch, add it in the patch that would actually use it
I thought I would put the only two functions that encapsulate the
knowledge of how vp_index is realted to cpu_index, in a single patch.
I'm now thinking of open-coding the iteration over cpus here and
directly look for cpu whose hyperv_vp_index() matches. Then that
knowledge will become encapsulated in a single place, and indeed, this
helper can go into another patch where it's used.
> also if qemu_get_cpu() were called from each CPU init,
> it would incur O(N^2) complexity, could you do without it?
It isn't called on hot paths (ATM it's called only when SINT routes are
created, which is at most one per cpu). I don't see a problem here.
> > @@ -105,7 +115,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> > }
> > sint_route->gsi = gsi;
> > sint_route->sint_ack_clb = sint_ack_clb;
> > - sint_route->vcpu_id = vcpu_id;
> > + sint_route->vcpu_id = vp_index;
> ^^^ - shouldn't it also be re-named?
Right, but vcpu_id on sint_route is replaced with X86CPU pointer in a
followup patch, so I wasn't sure if it was worth while to add more
churn...
>
> maybe split all renaming into separate patch ...
Part of the renaming will disappear eventually in the followup patches,
so I'm sure it's a good idea... Opinions?
> > +static int hyperv_init_vcpu(X86CPU *cpu)
> > +{
> > + if (cpu->hyperv_vpindex && !is_hv_vpindex_settable) {
> > + /*
> > + * the kernel doesn't support setting vp_index; assert that its value
> > + * is in sync
> > + */
> > + int ret;
> > + struct {
> > + struct kvm_msrs info;
> > + struct kvm_msr_entry entries[1];
> > + } msr_data = {
> > + .info.nmsrs = 1,
> > + .entries[0].index = HV_X64_MSR_VP_INDEX,
> > + };
> > +
> > + /*
> > + * handling errors from vcpu init at hotplug time is impossible, so
> > + * disallow cpu hotplug
> > + */
> > + MACHINE_GET_CLASS(current_machine)->hot_add_cpu = NULL;
> one shouldn't alter machine this way nor
> it would disable cpu hotplug, it would disable only cpu-add interface
> but device_add() would still work.
Understood, will rework as you suggest above.
> > + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> > + if (ret < 0) {
> when this can fail?
Dunno, but not checking for errors doesn't look good.
> > + return ret;
> > + }
> > + assert(ret == 1);
> > +
> > + if (msr_data.entries[0].data != hyperv_vp_index(cpu)) {
> > + fprintf(stderr, "kernel's vp_index != QEMU's vp_index\n");
> error_report()
OK (target/i386/kvm.c is already a mix of all possible styles of error
reporting, I must've followed a wrong one).
Thanks,
Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-29 9:53 ` Roman Kagan
@ 2017-06-29 11:53 ` Igor Mammedov
2017-06-29 13:10 ` Roman Kagan
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2017-06-29 11:53 UTC (permalink / raw)
To: Roman Kagan
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Thu, 29 Jun 2017 12:53:27 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Wed, Jun 28, 2017 at 04:47:43PM +0200, Igor Mammedov wrote:
> > On Wed, 21 Jun 2017 19:24:08 +0300
> > Roman Kagan <rkagan@virtuozzo.com> wrote:
> >
> > > Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> > > queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
> > > spec as a sequential number which can't exceed the maximum number of
> > > vCPUs per VM.
> > >
> > > It has to be owned by QEMU in order to preserve it across migration.
> > >
> > > However, the initial implementation in KVM didn't allow to set this
> > > msr, and KVM used its own notion of VP index. Fortunately, the way
> > > vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> > > equal to QEMU cpu_index.
> > >
> > > So choose cpu_index as the value for vp_index, and push that to KVM on
> > > kernels that support setting the msr. On older ones that don't, query
> > > the kernel value and assert that it's in sync with QEMU.
> > >
> > > Besides, since handling errors from vCPU init at hotplug time is
> > > impossible, disable vCPU hotplug.
> > proper place to check if cpu might be created is at
> > pc_cpu_pre_plug() where you can gracefully abort cpu creation process.
>
> Thanks for the suggestion, I'll rework it this way.
>
> > Also it's possible to create cold-plugged CPUs in out of order
> > sequence using
> > -device cpu-foo on CLI
> > will be hyperv kvm/guest side ok with it?
>
> On kernels that support setting HV_X64_MSR_VP_INDEX QEMU will
> synchronize all sides. On kernels that don't, if out-of-order creation
> results in vp_index mismatch between the kernel and QEMU, vcpu creation
> will fail.
And additional question,
what would happen if VM is started on host supporting VP index setting
and then migrated to a host without it?
>
> > > This patch also introduces accessor functions to wrap the mapping
> > > between a vCPU and its vp_index. Besides, a few variables are renamed
> > > to avoid confusion of vp_index with vcpu_id (== apic_id).
> > >
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > > ---
> > > v1 -> v2:
> > > - were patches 5, 6 in v1
> > > - move vp_index initialization to hyperv_init_vcpu
> > > - check capability before trying to set the msr
> > > - set the msr on the usual kvm_put_msrs path
> > > - disable cpu hotplug if msr is not settable
> > >
> > > target/i386/hyperv.h | 5 ++++-
> > > target/i386/hyperv.c | 16 +++++++++++++---
> > > target/i386/kvm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 68 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
> > > index 0c3b562..82f4757 100644
> > > --- a/target/i386/hyperv.h
> > > +++ b/target/i386/hyperv.h
> > > @@ -32,11 +32,14 @@ struct HvSintRoute {
> > >
> > > int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
> > >
> > > -HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> > > +HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
> > > HvSintAckClb sint_ack_clb);
> > >
> > > void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
> > >
> > > int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
> > >
> > > +uint32_t hyperv_vp_index(X86CPU *cpu);
> > > +X86CPU *hyperv_find_vcpu(uint32_t vp_index);
> > > +
> > > #endif
> > > diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
> > > index 227185c..4f57447 100644
> > > --- a/target/i386/hyperv.c
> > > +++ b/target/i386/hyperv.c
> > > @@ -16,6 +16,16 @@
> > > #include "hyperv.h"
> > > #include "hyperv_proto.h"
> > >
> > > +uint32_t hyperv_vp_index(X86CPU *cpu)
> > > +{
> > > + return CPU(cpu)->cpu_index;
> > > +}
> >
> >
> > > +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> > > +{
> > > + return X86_CPU(qemu_get_cpu(vp_index));
> > > +}
> > this helper isn't used in this patch, add it in the patch that would actually use it
>
> I thought I would put the only two functions that encapsulate the
> knowledge of how vp_index is realted to cpu_index, in a single patch.
>
> I'm now thinking of open-coding the iteration over cpus here and
> directly look for cpu whose hyperv_vp_index() matches. Then that
> knowledge will become encapsulated in a single place, and indeed, this
> helper can go into another patch where it's used.
>
> > also if qemu_get_cpu() were called from each CPU init,
> > it would incur O(N^2) complexity, could you do without it?
>
> It isn't called on hot paths (ATM it's called only when SINT routes are
> created, which is at most one per cpu). I don't see a problem here.
For what/where do you need this lookup?
>
> > > @@ -105,7 +115,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> > > }
> > > sint_route->gsi = gsi;
> > > sint_route->sint_ack_clb = sint_ack_clb;
> > > - sint_route->vcpu_id = vcpu_id;
> > > + sint_route->vcpu_id = vp_index;
> > ^^^ - shouldn't it also be re-named?
>
> Right, but vcpu_id on sint_route is replaced with X86CPU pointer in a
> followup patch, so I wasn't sure if it was worth while to add more
> churn...
>
> >
> > maybe split all renaming into separate patch ...
>
> Part of the renaming will disappear eventually in the followup patches,
> so I'm sure it's a good idea... Opinions?
I'd still spit. not to distract reviewers from
what this patch is actually tries to accomplish at least
>
> > > +static int hyperv_init_vcpu(X86CPU *cpu)
> > > +{
> > > + if (cpu->hyperv_vpindex && !is_hv_vpindex_settable) {
> > > + /*
> > > + * the kernel doesn't support setting vp_index; assert that its value
> > > + * is in sync
> > > + */
> > > + int ret;
> > > + struct {
> > > + struct kvm_msrs info;
> > > + struct kvm_msr_entry entries[1];
> > > + } msr_data = {
> > > + .info.nmsrs = 1,
> > > + .entries[0].index = HV_X64_MSR_VP_INDEX,
> > > + };
> > > +
> > > + /*
> > > + * handling errors from vcpu init at hotplug time is impossible, so
> > > + * disallow cpu hotplug
> > > + */
> > > + MACHINE_GET_CLASS(current_machine)->hot_add_cpu = NULL;
> > one shouldn't alter machine this way nor
> > it would disable cpu hotplug, it would disable only cpu-add interface
> > but device_add() would still work.
>
> Understood, will rework as you suggest above.
>
> > > + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> > > + if (ret < 0) {
> > when this can fail?
>
> Dunno, but not checking for errors doesn't look good.
Point of asking is to confirm that call shouldn't fail normally and
if it fails it's we can't recover and should die.
Looking at condition above !is_hv_vpindex_settable it looks like
call wouldn't fail. (i.e. it makes sure that qemu is running on
supported kernel)
>
> > > + return ret;
> > > + }
> > > + assert(ret == 1);
> > > +
> > > + if (msr_data.entries[0].data != hyperv_vp_index(cpu)) {
> > > + fprintf(stderr, "kernel's vp_index != QEMU's vp_index\n");
> > error_report()
>
> OK (target/i386/kvm.c is already a mix of all possible styles of error
> reporting, I must've followed a wrong one).
>
> Thanks,
> Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-29 11:53 ` Igor Mammedov
@ 2017-06-29 13:10 ` Roman Kagan
2017-06-29 14:39 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Roman Kagan @ 2017-06-29 13:10 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Thu, Jun 29, 2017 at 01:53:29PM +0200, Igor Mammedov wrote:
> On Thu, 29 Jun 2017 12:53:27 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
>
> > On Wed, Jun 28, 2017 at 04:47:43PM +0200, Igor Mammedov wrote:
> > > On Wed, 21 Jun 2017 19:24:08 +0300
> > > Roman Kagan <rkagan@virtuozzo.com> wrote:
> > >
> > > > Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> > > > queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
> > > > spec as a sequential number which can't exceed the maximum number of
> > > > vCPUs per VM.
> > > >
> > > > It has to be owned by QEMU in order to preserve it across migration.
> > > >
> > > > However, the initial implementation in KVM didn't allow to set this
> > > > msr, and KVM used its own notion of VP index. Fortunately, the way
> > > > vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> > > > equal to QEMU cpu_index.
> > > >
> > > > So choose cpu_index as the value for vp_index, and push that to KVM on
> > > > kernels that support setting the msr. On older ones that don't, query
> > > > the kernel value and assert that it's in sync with QEMU.
> > > >
> > > > Besides, since handling errors from vCPU init at hotplug time is
> > > > impossible, disable vCPU hotplug.
> > > proper place to check if cpu might be created is at
> > > pc_cpu_pre_plug() where you can gracefully abort cpu creation process.
> >
> > Thanks for the suggestion, I'll rework it this way.
> >
> > > Also it's possible to create cold-plugged CPUs in out of order
> > > sequence using
> > > -device cpu-foo on CLI
> > > will be hyperv kvm/guest side ok with it?
> >
> > On kernels that support setting HV_X64_MSR_VP_INDEX QEMU will
> > synchronize all sides. On kernels that don't, if out-of-order creation
> > results in vp_index mismatch between the kernel and QEMU, vcpu creation
> > will fail.
>
> And additional question,
> what would happen if VM is started on host supporting VP index setting
> and then migrated to a host without it?
The destination QEMU will attempt to initialize vCPUs, and if that
fails (e.g. due to vp_index mismatch), the migration will be aborted and
the source VM will continue running.
If the destination QEMU is old, too, there's a chance that vp_index will
change. Then we just keep the fingers crossed that the guest doesn't
notice (this is the behavior we have now).
> > > > +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> > > > +{
> > > > + return X86_CPU(qemu_get_cpu(vp_index));
> > > > +}
> > > this helper isn't used in this patch, add it in the patch that would actually use it
> >
> > I thought I would put the only two functions that encapsulate the
> > knowledge of how vp_index is realted to cpu_index, in a single patch.
> >
> > I'm now thinking of open-coding the iteration over cpus here and
> > directly look for cpu whose hyperv_vp_index() matches. Then that
> > knowledge will become encapsulated in a single place, and indeed, this
> > helper can go into another patch where it's used.
> >
> > > also if qemu_get_cpu() were called from each CPU init,
> > > it would incur O(N^2) complexity, could you do without it?
> >
> > It isn't called on hot paths (ATM it's called only when SINT routes are
> > created, which is at most one per cpu). I don't see a problem here.
> For what/where do you need this lookup?
The guest configures devices to signal their events with synthetic
interrupts on specific cpus, identified by vp_index. When we receive
such a request we look up the cpu and set up a SINT route to be able to
deliver interrupts to its synic.
Or did I misunderstand the question perhaps?
> > > maybe split all renaming into separate patch ...
> >
> > Part of the renaming will disappear eventually in the followup patches,
> > so I'm sure it's a good idea... Opinions?
> I'd still spit. not to distract reviewers from
> what this patch is actually tries to accomplish at least
OK, makes sense.
> > > > + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> > > > + if (ret < 0) {
> > > when this can fail?
> >
> > Dunno, but not checking for errors doesn't look good.
> Point of asking is to confirm that call shouldn't fail normally and
> if it fails it's we can't recover and should die.
>
> Looking at condition above !is_hv_vpindex_settable it looks like
> call wouldn't fail. (i.e. it makes sure that qemu is running on
> supported kernel)
No, I do KVM_GET_MSRS here, which is supported since this msr was
introduced.
But actually you've spot a bug which I meant to fix but forgot: the code
in hyperv_handle_properties should fail if a particular hyperv feature
is requested but the corresponding msr isn't found among reported via
KVM_GET_MSR_INDEX_LIST. ATM this is only done for some features but not
all.
I'll try to fix that; once done, cpu->hyperv_vpindex here will make sure
this msr is gettable.
Thanks,
Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-29 13:10 ` Roman Kagan
@ 2017-06-29 14:39 ` Igor Mammedov
2017-06-29 17:31 ` Roman Kagan
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2017-06-29 14:39 UTC (permalink / raw)
To: Roman Kagan
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Thu, 29 Jun 2017 16:10:20 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Thu, Jun 29, 2017 at 01:53:29PM +0200, Igor Mammedov wrote:
> > On Thu, 29 Jun 2017 12:53:27 +0300
> > Roman Kagan <rkagan@virtuozzo.com> wrote:
> >
> > > On Wed, Jun 28, 2017 at 04:47:43PM +0200, Igor Mammedov wrote:
> > > > On Wed, 21 Jun 2017 19:24:08 +0300
> > > > Roman Kagan <rkagan@virtuozzo.com> wrote:
> > > >
> > > > > Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> > > > > queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
> > > > > spec as a sequential number which can't exceed the maximum number of
> > > > > vCPUs per VM.
> > > > >
> > > > > It has to be owned by QEMU in order to preserve it across migration.
> > > > >
> > > > > However, the initial implementation in KVM didn't allow to set this
> > > > > msr, and KVM used its own notion of VP index. Fortunately, the way
> > > > > vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> > > > > equal to QEMU cpu_index.
> > > > >
> > > > > So choose cpu_index as the value for vp_index, and push that to KVM on
> > > > > kernels that support setting the msr. On older ones that don't, query
> > > > > the kernel value and assert that it's in sync with QEMU.
> > > > >
> > > > > Besides, since handling errors from vCPU init at hotplug time is
> > > > > impossible, disable vCPU hotplug.
> > > > proper place to check if cpu might be created is at
> > > > pc_cpu_pre_plug() where you can gracefully abort cpu creation process.
> > >
> > > Thanks for the suggestion, I'll rework it this way.
> > >
> > > > Also it's possible to create cold-plugged CPUs in out of order
> > > > sequence using
> > > > -device cpu-foo on CLI
> > > > will be hyperv kvm/guest side ok with it?
> > >
> > > On kernels that support setting HV_X64_MSR_VP_INDEX QEMU will
> > > synchronize all sides. On kernels that don't, if out-of-order creation
> > > results in vp_index mismatch between the kernel and QEMU, vcpu creation
> > > will fail.
> >
> > And additional question,
> > what would happen if VM is started on host supporting VP index setting
> > and then migrated to a host without it?
>
> The destination QEMU will attempt to initialize vCPUs, and if that
> fails (e.g. due to vp_index mismatch), the migration will be aborted and
> the source VM will continue running.
>
> If the destination QEMU is old, too, there's a chance that vp_index will
> change. Then we just keep the fingers crossed that the guest doesn't
> notice (this is the behavior we have now).
on source, putting in migration stream a flag that setting HV_X64_MSR_VP_INDEX
is in use, should prevent migration to the old destination or new destination but
without kernel support.
It also might make sense to disable feature for old machine types
so new->old migration would work as it used to be even if
destination kernel supports feature.
> > > > > +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> > > > > +{
> > > > > + return X86_CPU(qemu_get_cpu(vp_index));
> > > > > +}
> > > > this helper isn't used in this patch, add it in the patch that would actually use it
> > >
> > > I thought I would put the only two functions that encapsulate the
> > > knowledge of how vp_index is realted to cpu_index, in a single patch.
> > >
> > > I'm now thinking of open-coding the iteration over cpus here and
> > > directly look for cpu whose hyperv_vp_index() matches. Then that
> > > knowledge will become encapsulated in a single place, and indeed, this
> > > helper can go into another patch where it's used.
> > >
> > > > also if qemu_get_cpu() were called from each CPU init,
> > > > it would incur O(N^2) complexity, could you do without it?
> > >
> > > It isn't called on hot paths (ATM it's called only when SINT routes are
> > > created, which is at most one per cpu). I don't see a problem here.
> > For what/where do you need this lookup?
>
> The guest configures devices to signal their events with synthetic
> interrupts on specific cpus, identified by vp_index. When we receive
> such a request we look up the cpu and set up a SINT route to be able to
> deliver interrupts to its synic.
>
> Or did I misunderstand the question perhaps?
since there is 1:1 mapping between synic:vp_index and
vp_index is dense interval of [0..maxcpus),
I'd suggest to maintain internal synic map where vp_index
could be used as index in array to fetch addressed synic.
look for local_apics as example.
[...]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC Roman Kagan
@ 2017-06-29 15:05 ` Igor Mammedov
2017-06-29 17:51 ` Roman Kagan
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2017-06-29 15:05 UTC (permalink / raw)
To: Roman Kagan
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Wed, 21 Jun 2017 19:24:15 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> Make Hyper-V SynIC a device which is attached as a child to X86CPU. For
> now it only makes SynIC visibile in the qom hierarchy, and maintains its
> internal fields in sync with the respecitve msrs of the parent cpu (the
> fields will be used in followup patches).
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> v1 -> v2:
> - was patch 13 in v1
> - drop unnecessary QOM properties (but keep the fields)
> - move KVM_CAP_HYPERV_SYNIC setting and SynIC creation to
> hyperv_init_vcpu
>
> target/i386/hyperv.h | 4 ++
> target/i386/hyperv.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++-
> target/i386/kvm.c | 14 ++++++-
> target/i386/machine.c | 9 ++++
> 4 files changed, 134 insertions(+), 4 deletions(-)
>
> diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
> index af5fc05..20bbd7b 100644
> --- a/target/i386/hyperv.h
> +++ b/target/i386/hyperv.h
> @@ -34,4 +34,8 @@ int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
> uint32_t hyperv_vp_index(X86CPU *cpu);
> X86CPU *hyperv_find_vcpu(uint32_t vp_index);
>
> +void hyperv_synic_add(X86CPU *cpu);
> +void hyperv_synic_reset(X86CPU *cpu);
> +void hyperv_synic_update(X86CPU *cpu);
> +
> #endif
> diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
> index 012c79d..eff612c 100644
> --- a/target/i386/hyperv.c
> +++ b/target/i386/hyperv.c
> @@ -13,12 +13,27 @@
>
> #include "qemu/osdep.h"
> #include "qemu/main-loop.h"
> +#include "qapi/error.h"
> +#include "hw/qdev-properties.h"
> #include "hyperv.h"
> #include "hyperv_proto.h"
>
> +typedef struct SynICState {
> + DeviceState parent_obj;
> +
> + X86CPU *cpu;
> +
> + bool enabled;
> + hwaddr msg_page_addr;
> + hwaddr evt_page_addr;
> +} SynICState;
> +
> +#define TYPE_SYNIC "hyperv-synic"
> +#define SYNIC(obj) OBJECT_CHECK(SynICState, (obj), TYPE_SYNIC)
> +
> struct HvSintRoute {
> uint32_t sint;
> - X86CPU *cpu;
> + SynICState *synic;
> int gsi;
> EventNotifier sint_set_notifier;
> EventNotifier sint_ack_notifier;
> @@ -37,6 +52,37 @@ X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> return X86_CPU(qemu_get_cpu(vp_index));
> }
>
> +static SynICState *get_synic(X86CPU *cpu)
> +{
> + SynICState *synic =
> + SYNIC(object_resolve_path_component(OBJECT(cpu), "synic"));
> + assert(synic);
> + return synic;
> +}
> +
> +static void synic_update_msg_page_addr(SynICState *synic)
> +{
> + uint64_t msr = synic->cpu->env.msr_hv_synic_msg_page;
> + hwaddr new_addr = (msr & HV_SIMP_ENABLE) ? (msr & TARGET_PAGE_MASK) : 0;
> +
> + synic->msg_page_addr = new_addr;
> +}
> +
> +static void synic_update_evt_page_addr(SynICState *synic)
> +{
> + uint64_t msr = synic->cpu->env.msr_hv_synic_evt_page;
> + hwaddr new_addr = (msr & HV_SIEFP_ENABLE) ? (msr & TARGET_PAGE_MASK) : 0;
> +
> + synic->evt_page_addr = new_addr;
> +}
> +
> +static void synic_update(SynICState *synic)
> +{
> + synic->enabled = synic->cpu->env.msr_hv_synic_control & HV_SYNIC_ENABLE;
> + synic_update_msg_page_addr(synic);
> + synic_update_evt_page_addr(synic);
> +}
> +
> int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
> {
> CPUX86State *env = &cpu->env;
> @@ -65,6 +111,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
> default:
> return -1;
> }
> + synic_update(get_synic(cpu));
> return 0;
> case KVM_EXIT_HYPERV_HCALL: {
> uint16_t code;
> @@ -95,6 +142,7 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
> HvSintAckClb sint_ack_clb,
> void *sint_ack_clb_data)
> {
> + SynICState *synic;
> HvSintRoute *sint_route;
> EventNotifier *ack_notifier;
> int r, gsi;
> @@ -105,6 +153,8 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
> return NULL;
> }
>
> + synic = get_synic(cpu);
> +
> sint_route = g_new0(HvSintRoute, 1);
> r = event_notifier_init(&sint_route->sint_set_notifier, false);
> if (r) {
> @@ -135,7 +185,7 @@ HvSintRoute *hyperv_sint_route_new(uint32_t vp_index, uint32_t sint,
> sint_route->gsi = gsi;
> sint_route->sint_ack_clb = sint_ack_clb;
> sint_route->sint_ack_clb_data = sint_ack_clb_data;
> - sint_route->cpu = cpu;
> + sint_route->synic = synic;
> sint_route->sint = sint;
> sint_route->refcount = 1;
>
> @@ -189,3 +239,60 @@ int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route)
> {
> return event_notifier_set(&sint_route->sint_set_notifier);
> }
> +
> +static void synic_realize(DeviceState *dev, Error **errp)
> +{
> + Object *obj = OBJECT(dev);
> + SynICState *synic = SYNIC(dev);
> +
> + synic->cpu = X86_CPU(obj->parent);
usually devices shouldn't access parent this way
[...]
> +void hyperv_synic_add(X86CPU *cpu)
this helper is called by/from parent so something like below should do
> +{
> + Object *obj;
> +
> + obj = object_new(TYPE_SYNIC);
> + object_property_add_child(OBJECT(cpu), "synic", obj, &error_abort);
> + object_unref(obj);
SynICState *synic = SYNIC(obj)
synic->cpu = cpu;
or even make synic->cpu a link (object_property_add_link) and set it
here, benefit will be when synic is introspected via QOM it will show
that it refers back/uses the cpu + proper reference counting of CPU object.
> + object_property_set_bool(obj, true, "realized", &error_abort);
> +}
> +
> +void hyperv_synic_reset(X86CPU *cpu)
> +{
> + device_reset(DEVICE(get_synic(cpu)));
> +}
> +
> +void hyperv_synic_update(X86CPU *cpu)
> +{
> + synic_update(get_synic(cpu));
> +}
> +
> +static const TypeInfo synic_type_info = {
> + .name = TYPE_SYNIC,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(SynICState),
> + .class_init = synic_class_init,
> +};
> +
> +static void synic_register_types(void)
> +{
> + type_register_static(&synic_type_info);
> +}
> +
> +type_init(synic_register_types)
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 9bf7f08..eaa2df3 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -648,8 +648,7 @@ static int hyperv_handle_properties(CPUState *cs)
> env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
> }
> if (cpu->hyperv_synic) {
> - if (!has_msr_hv_synic ||
> - kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) {
> + if (!has_msr_hv_synic) {
> fprintf(stderr, "Hyper-V SynIC is not supported by kernel\n");
> return -ENOSYS;
> }
> @@ -700,6 +699,15 @@ static int hyperv_init_vcpu(X86CPU *cpu)
> }
> }
>
> + if (cpu->hyperv_synic) {
> + if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
> + fprintf(stderr, "failed to enable Hyper-V SynIC\n");
> + return -ENOSYS;
> + }
> +
> + hyperv_synic_add(cpu);
is synic KVM specific or may it work with TCG accel?
in either case, looks like hyperv_synic_add() should be called from
x86_cpu_realizefn(), the same like we do with APIC creating it
depending feature being enabled.
> + }
> +
> return 0;
> }
>
> @@ -1084,6 +1092,8 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
> for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) {
> env->msr_hv_synic_sint[i] = HV_SINT_MASKED;
> }
> +
> + hyperv_synic_reset(cpu);
ditto, could go to x86_cpu_machine_reset_cb()
also calling it unconditionally will crash QEMU if
get_synic() returns NULL (i.e. when feature is not enabled).
> }
> }
>
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index ded5e34..90cc3a9 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -7,6 +7,7 @@
> #include "hw/i386/pc.h"
> #include "hw/isa/isa.h"
> #include "migration/cpu.h"
> +#include "hyperv.h"
>
> #include "sysemu/kvm.h"
>
> @@ -629,11 +630,19 @@ static bool hyperv_synic_enable_needed(void *opaque)
> return false;
> }
>
> +static int hyperv_synic_post_load(void *opaque, int version_id)
> +{
> + X86CPU *cpu = opaque;
> + hyperv_synic_update(cpu);
> + return 0;
> +}
> +
> static const VMStateDescription vmstate_msr_hyperv_synic = {
> .name = "cpu/msr_hyperv_synic",
> .version_id = 1,
> .minimum_version_id = 1,
> .needed = hyperv_synic_enable_needed,
> + .post_load = hyperv_synic_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINT64(env.msr_hv_synic_control, X86CPU),
> VMSTATE_UINT64(env.msr_hv_synic_evt_page, X86CPU),
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
` (21 preceding siblings ...)
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 23/23] hyperv: update copyright notices Roman Kagan
@ 2017-06-29 15:20 ` Igor Mammedov
2017-06-29 17:58 ` Roman Kagan
22 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2017-06-29 15:20 UTC (permalink / raw)
To: Roman Kagan
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Denis V . Lunev,
Evgeny Yakovlev
On Wed, 21 Jun 2017 19:24:01 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> This series applies miscellaneous fixes and enhancements to Hyper-V
> emulation code in QEMU, and lays out the ground for VMBus devices.
Series is a big random mix of cleanup/refactoring/new features
which is hard to review due to mixed content of above
within patches as well inter them.
I suggest to split series in at least in 2
1: make cleanups/refactoring first
and on top of this
2: add new features
* hyperv_find_vcpu() with expensive lookup and related renaming
might be not needed at all if local_apics[12/23] is used.
*pls add in cover letter a link to public git repo where
it would be possible to pull from for testing/review.
PS:
if renaming, do it consistently.
ex:
vcpu_id -> vp_index touches only hyperv.c
but leaves old naming in user of kvm_hv_sint_route_create(): hyperv_testdev.c
> v1 -> v2:
> - drop the already merged patch
> - split and rework SINTx and SVERSION msrs init
> - factor out hyperv vcpu init to a function
> - rework vp_index management
> - distinguish kvm-only (== legacy) mode for SynIC
> - use new capabilities recently submitted to KVM
> - add compat logic for SynIC
> - drop workaround for KVM zeroing SynIC pages
> - minor fixes according to comments
>
> Evgeny Yakovlev (1):
> hyperv: set partition-wide MSRs only on first vcpu
>
> Roman Kagan (22):
> hyperv: add header with protocol definitions
> update-linux-headers: prepare for hyperv.h removal
> hyperv: ensure SINTx msrs are reset properly
> hyperv: make SynIC version msr constant
> [not to commit] add new hyperv-related caps
> hyperv: ensure VP index equal to QEMU cpu_index
> hyperv_testdev: refactor for readability
> hyperv: cosmetic: g_malloc -> g_new
> hyperv: synic: only setup ack notifier if there's a callback
> hyperv: allow passing arbitrary data to sint ack callback
> hyperv: address HvSintRoute by X86CPU pointer
> hyperv: make HvSintRoute reference-counted
> hyperv: qom-ify SynIC
> hyperv: block SynIC use in QEMU in incompatible configurations
> hyperv: make overlay pages for SynIC
> hyperv: add synic message delivery
> hyperv: add synic event flag signaling
> hyperv: process SIGNAL_EVENT hypercall
> hyperv: process POST_MESSAGE hypercall
> hyperv_testdev: add SynIC message and event testmodes
> MAINTAINERS: add myself and eyakovlev@ for hyperv*
> hyperv: update copyright notices
>
> include/hw/i386/pc.h | 5 +
> linux-headers/linux/kvm.h | 2 +
> target/i386/cpu.h | 16 +-
> target/i386/hyperv.h | 40 ++-
> target/i386/hyperv_proto.h | 257 ++++++++++++++++
> hw/misc/hyperv_testdev.c | 267 +++++++++++++----
> target/i386/cpu.c | 4 +-
> target/i386/hyperv.c | 634 ++++++++++++++++++++++++++++++++++++++--
> target/i386/kvm.c | 167 +++++++----
> target/i386/machine.c | 24 +-
> MAINTAINERS | 7 +
> scripts/update-linux-headers.sh | 4 +-
> 12 files changed, 1256 insertions(+), 171 deletions(-)
> create mode 100644 target/i386/hyperv_proto.h
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index
2017-06-29 14:39 ` Igor Mammedov
@ 2017-06-29 17:31 ` Roman Kagan
0 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-29 17:31 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Thu, Jun 29, 2017 at 04:39:00PM +0200, Igor Mammedov wrote:
> On Thu, 29 Jun 2017 16:10:20 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
>
> > On Thu, Jun 29, 2017 at 01:53:29PM +0200, Igor Mammedov wrote:
> > > On Thu, 29 Jun 2017 12:53:27 +0300
> > > Roman Kagan <rkagan@virtuozzo.com> wrote:
> > >
> > > > On Wed, Jun 28, 2017 at 04:47:43PM +0200, Igor Mammedov wrote:
> > > > > On Wed, 21 Jun 2017 19:24:08 +0300
> > > > > Roman Kagan <rkagan@virtuozzo.com> wrote:
> > > > >
> > > > > > Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> > > > > > queried by the guest via HV_X64_MSR_VP_INDEX msr. It is defined by the
> > > > > > spec as a sequential number which can't exceed the maximum number of
> > > > > > vCPUs per VM.
> > > > > >
> > > > > > It has to be owned by QEMU in order to preserve it across migration.
> > > > > >
> > > > > > However, the initial implementation in KVM didn't allow to set this
> > > > > > msr, and KVM used its own notion of VP index. Fortunately, the way
> > > > > > vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> > > > > > equal to QEMU cpu_index.
> > > > > >
> > > > > > So choose cpu_index as the value for vp_index, and push that to KVM on
> > > > > > kernels that support setting the msr. On older ones that don't, query
> > > > > > the kernel value and assert that it's in sync with QEMU.
> > > > > >
> > > > > > Besides, since handling errors from vCPU init at hotplug time is
> > > > > > impossible, disable vCPU hotplug.
> > > > > proper place to check if cpu might be created is at
> > > > > pc_cpu_pre_plug() where you can gracefully abort cpu creation process.
> > > >
> > > > Thanks for the suggestion, I'll rework it this way.
> > > >
> > > > > Also it's possible to create cold-plugged CPUs in out of order
> > > > > sequence using
> > > > > -device cpu-foo on CLI
> > > > > will be hyperv kvm/guest side ok with it?
> > > >
> > > > On kernels that support setting HV_X64_MSR_VP_INDEX QEMU will
> > > > synchronize all sides. On kernels that don't, if out-of-order creation
> > > > results in vp_index mismatch between the kernel and QEMU, vcpu creation
> > > > will fail.
> > >
> > > And additional question,
> > > what would happen if VM is started on host supporting VP index setting
> > > and then migrated to a host without it?
> >
> > The destination QEMU will attempt to initialize vCPUs, and if that
> > fails (e.g. due to vp_index mismatch), the migration will be aborted and
> > the source VM will continue running.
> >
> > If the destination QEMU is old, too, there's a chance that vp_index will
> > change. Then we just keep the fingers crossed that the guest doesn't
> > notice (this is the behavior we have now).
> on source, putting in migration stream a flag that setting HV_X64_MSR_VP_INDEX
> is in use, should prevent migration to the old destination or new destination but
> without kernel support.
These are different cases. New destination QEMU can verify that all
vcpus have the desired vp_index even if it can't set it, so in this case
vp_index migration is even reliable.
Old QEMU didn't bother so it potentially can confuse the guest. But
we're unaware of this ever happening in the past, probably because the
existing users of synic are only in-kvm synic timers which don't depend
on vp_index.
> It also might make sense to disable feature for old machine types
> so new->old migration would work as it used to be even if
> destination kernel supports feature.
I'm not sure it's worth the effort, especially since other patches in
the series introduce "in-kvm-only" flag in SynIC, which is "on" for old
machine types. So eventually the migration to/from an old QEMU will
only be possible for configurations with only in-kvm synic users, where
we hope vp_index not to matter.
> > > > > > +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> > > > > > +{
> > > > > > + return X86_CPU(qemu_get_cpu(vp_index));
> > > > > > +}
> > > > > this helper isn't used in this patch, add it in the patch that would actually use it
> > > >
> > > > I thought I would put the only two functions that encapsulate the
> > > > knowledge of how vp_index is realted to cpu_index, in a single patch.
> > > >
> > > > I'm now thinking of open-coding the iteration over cpus here and
> > > > directly look for cpu whose hyperv_vp_index() matches. Then that
> > > > knowledge will become encapsulated in a single place, and indeed, this
> > > > helper can go into another patch where it's used.
> > > >
> > > > > also if qemu_get_cpu() were called from each CPU init,
> > > > > it would incur O(N^2) complexity, could you do without it?
> > > >
> > > > It isn't called on hot paths (ATM it's called only when SINT routes are
> > > > created, which is at most one per cpu). I don't see a problem here.
> > > For what/where do you need this lookup?
> >
> > The guest configures devices to signal their events with synthetic
> > interrupts on specific cpus, identified by vp_index. When we receive
> > such a request we look up the cpu and set up a SINT route to be able to
> > deliver interrupts to its synic.
> >
> > Or did I misunderstand the question perhaps?
> since there is 1:1 mapping between synic:vp_index and
> vp_index is dense interval of [0..maxcpus),
> I'd suggest to maintain internal synic map where vp_index
> could be used as index in array to fetch addressed synic.
Ah, I see. I wonder why qemu_get_cpu() itself isn't implemented this
way?
Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC
2017-06-29 15:05 ` Igor Mammedov
@ 2017-06-29 17:51 ` Roman Kagan
2017-07-07 12:22 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Roman Kagan @ 2017-06-29 17:51 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Evgeny Yakovlev,
Denis V . Lunev
On Thu, Jun 29, 2017 at 05:05:46PM +0200, Igor Mammedov wrote:
> On Wed, 21 Jun 2017 19:24:15 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
> > +static void synic_realize(DeviceState *dev, Error **errp)
> > +{
> > + Object *obj = OBJECT(dev);
> > + SynICState *synic = SYNIC(dev);
> > +
> > + synic->cpu = X86_CPU(obj->parent);
> usually devices shouldn't access parent this way
>
> [...]
> > +void hyperv_synic_add(X86CPU *cpu)
> this helper is called by/from parent so something like below should do
>
> > +{
> > + Object *obj;
> > +
> > + obj = object_new(TYPE_SYNIC);
> > + object_property_add_child(OBJECT(cpu), "synic", obj, &error_abort);
> > + object_unref(obj);
>
> SynICState *synic = SYNIC(obj)
> synic->cpu = cpu;
Indeed, this is better.
> or even make synic->cpu a link (object_property_add_link) and set it
> here, benefit will be when synic is introspected via QOM it will show
> that it refers back/uses the cpu + proper reference counting of CPU object.
Good point, will look into it, thanks.
> > + if (cpu->hyperv_synic) {
> > + if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
> > + fprintf(stderr, "failed to enable Hyper-V SynIC\n");
> > + return -ENOSYS;
> > + }
> > +
> > + hyperv_synic_add(cpu);
> is synic KVM specific or may it work with TCG accel?
No, it's exclusively KVM. Actually most of it sits in the kernel.
> in either case, looks like hyperv_synic_add() should be called from
> x86_cpu_realizefn(), the same like we do with APIC creating it
> depending feature being enabled.
I'm not sure I understand the reason, in view of it being exclusively
KVM.
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1084,6 +1092,8 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
> > for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) {
> > env->msr_hv_synic_sint[i] = HV_SINT_MASKED;
> > }
> > +
> > + hyperv_synic_reset(cpu);
> ditto, could go to x86_cpu_machine_reset_cb()
> also calling it unconditionally will crash QEMU if
> get_synic() returns NULL (i.e. when feature is not enabled).
The context is not visibile in the patch, but this is actually called
inside
if (cpu->hyperv_synic) {
...
}
so no, it won't return NULL.
Thanks,
Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements
2017-06-29 15:20 ` [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Igor Mammedov
@ 2017-06-29 17:58 ` Roman Kagan
0 siblings, 0 replies; 36+ messages in thread
From: Roman Kagan @ 2017-06-29 17:58 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Denis V . Lunev,
Evgeny Yakovlev
On Thu, Jun 29, 2017 at 05:20:30PM +0200, Igor Mammedov wrote:
> On Wed, 21 Jun 2017 19:24:01 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
>
> > This series applies miscellaneous fixes and enhancements to Hyper-V
> > emulation code in QEMU, and lays out the ground for VMBus devices.
>
> Series is a big random mix of cleanup/refactoring/new features
> which is hard to review due to mixed content of above
> within patches as well inter them.
>
> I suggest to split series in at least in 2
> 1: make cleanups/refactoring first
> and on top of this
> 2: add new features
Indeed, I started to realize this. I'll try to re-split better when
respinning.
> * hyperv_find_vcpu() with expensive lookup and related renaming
> might be not needed at all if local_apics[12/23] is used.
>
> *pls add in cover letter a link to public git repo where
> it would be possible to pull from for testing/review.
I'm in the process of setting it up, will include it next time.
> PS:
> if renaming, do it consistently.
> ex:
> vcpu_id -> vp_index touches only hyperv.c
> but leaves old naming in user of kvm_hv_sint_route_create(): hyperv_testdev.c
I strived for eventual consistency: no vcpu_id is left after the last
patch in the series. But it's probably harder to review patch by patch.
So I'll redo the way you suggest.
Thanks,
Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC
2017-06-29 17:51 ` Roman Kagan
@ 2017-07-07 12:22 ` Igor Mammedov
2017-07-07 12:47 ` Roman Kagan
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2017-07-07 12:22 UTC (permalink / raw)
To: Roman Kagan
Cc: Denis V . Lunev, Paolo Bonzini, Evgeny Yakovlev, qemu-devel,
Eduardo Habkost
On Thu, 29 Jun 2017 20:51:01 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Thu, Jun 29, 2017 at 05:05:46PM +0200, Igor Mammedov wrote:
> > On Wed, 21 Jun 2017 19:24:15 +0300
> > Roman Kagan <rkagan@virtuozzo.com> wrote:
[...]
>
> > > + if (cpu->hyperv_synic) {
> > > + if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
> > > + fprintf(stderr, "failed to enable Hyper-V SynIC\n");
> > > + return -ENOSYS;
> > > + }
> > > +
> > > + hyperv_synic_add(cpu);
> > is synic KVM specific or may it work with TCG accel?
>
> No, it's exclusively KVM. Actually most of it sits in the kernel.
>
> > in either case, looks like hyperv_synic_add() should be called from
> > x86_cpu_realizefn(), the same like we do with APIC creating it
> > depending feature being enabled.
>
> I'm not sure I understand the reason, in view of it being exclusively
> KVM.
couldn't synic be emulated (if missing QEMU part is written) in TCG mode
or it relies on specific KVM features?
[...]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC
2017-07-07 12:22 ` Igor Mammedov
@ 2017-07-07 12:47 ` Roman Kagan
2017-07-07 13:27 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Roman Kagan @ 2017-07-07 12:47 UTC (permalink / raw)
To: Igor Mammedov
Cc: Denis V . Lunev, Paolo Bonzini, Evgeny Yakovlev, qemu-devel,
Eduardo Habkost
On Fri, Jul 07, 2017 at 02:22:20PM +0200, Igor Mammedov wrote:
> On Thu, 29 Jun 2017 20:51:01 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
>
> > On Thu, Jun 29, 2017 at 05:05:46PM +0200, Igor Mammedov wrote:
> > > On Wed, 21 Jun 2017 19:24:15 +0300
> > > Roman Kagan <rkagan@virtuozzo.com> wrote:
> [...]
> >
> > > > + if (cpu->hyperv_synic) {
> > > > + if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
> > > > + fprintf(stderr, "failed to enable Hyper-V SynIC\n");
> > > > + return -ENOSYS;
> > > > + }
> > > > +
> > > > + hyperv_synic_add(cpu);
> > > is synic KVM specific or may it work with TCG accel?
> >
> > No, it's exclusively KVM. Actually most of it sits in the kernel.
> >
> > > in either case, looks like hyperv_synic_add() should be called from
> > > x86_cpu_realizefn(), the same like we do with APIC creating it
> > > depending feature being enabled.
> >
> > I'm not sure I understand the reason, in view of it being exclusively
> > KVM.
> couldn't synic be emulated (if missing QEMU part is written) in TCG mode
> or it relies on specific KVM features?
At the moment it depends on KVM and, in particular, relies on lapic
being in KVM. I guess with certain effort it can be done in TCG too,
but we didn't even consider this so far, and have no plans for it.
Roman.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC
2017-07-07 12:47 ` Roman Kagan
@ 2017-07-07 13:27 ` Igor Mammedov
0 siblings, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2017-07-07 13:27 UTC (permalink / raw)
To: Roman Kagan
Cc: Denis V . Lunev, Paolo Bonzini, Evgeny Yakovlev, qemu-devel,
Eduardo Habkost
On Fri, 7 Jul 2017 15:47:19 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Fri, Jul 07, 2017 at 02:22:20PM +0200, Igor Mammedov wrote:
> > On Thu, 29 Jun 2017 20:51:01 +0300
> > Roman Kagan <rkagan@virtuozzo.com> wrote:
> >
> > > On Thu, Jun 29, 2017 at 05:05:46PM +0200, Igor Mammedov wrote:
> > > > On Wed, 21 Jun 2017 19:24:15 +0300
> > > > Roman Kagan <rkagan@virtuozzo.com> wrote:
> > [...]
> > >
> > > > > + if (cpu->hyperv_synic) {
> > > > > + if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
> > > > > + fprintf(stderr, "failed to enable Hyper-V SynIC\n");
> > > > > + return -ENOSYS;
> > > > > + }
> > > > > +
> > > > > + hyperv_synic_add(cpu);
> > > > is synic KVM specific or may it work with TCG accel?
> > >
> > > No, it's exclusively KVM. Actually most of it sits in the kernel.
> > >
> > > > in either case, looks like hyperv_synic_add() should be called from
> > > > x86_cpu_realizefn(), the same like we do with APIC creating it
> > > > depending feature being enabled.
> > >
> > > I'm not sure I understand the reason, in view of it being exclusively
> > > KVM.
> > couldn't synic be emulated (if missing QEMU part is written) in TCG mode
> > or it relies on specific KVM features?
>
> At the moment it depends on KVM and, in particular, relies on lapic
> being in KVM. I guess with certain effort it can be done in TCG too,
> but we didn't even consider this so far, and have no plans for it.
we would avoid rewriting synic code if you can keep synic parts
that could be reused by TCG outside of kvm specific files.
Refactoring to qom/device and moving that code in separate file
is a good time to start doing it and keeping kvm related parts isolated/minimal
should help in future when TCG synic model could be written.
(but I don't insist on it).
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2017-07-07 13:27 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-21 16:24 [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 01/23] hyperv: add header with protocol definitions Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 02/23] update-linux-headers: prepare for hyperv.h removal Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 03/23] hyperv: set partition-wide MSRs only on first vcpu Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 04/23] hyperv: ensure SINTx msrs are reset properly Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 05/23] hyperv: make SynIC version msr constant Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 06/23] [not to commit] add new hyperv-related caps Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index Roman Kagan
2017-06-28 14:47 ` Igor Mammedov
2017-06-29 9:53 ` Roman Kagan
2017-06-29 11:53 ` Igor Mammedov
2017-06-29 13:10 ` Roman Kagan
2017-06-29 14:39 ` Igor Mammedov
2017-06-29 17:31 ` Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 08/23] hyperv_testdev: refactor for readability Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 09/23] hyperv: cosmetic: g_malloc -> g_new Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 10/23] hyperv: synic: only setup ack notifier if there's a callback Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 11/23] hyperv: allow passing arbitrary data to sint ack callback Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 12/23] hyperv: address HvSintRoute by X86CPU pointer Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 13/23] hyperv: make HvSintRoute reference-counted Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC Roman Kagan
2017-06-29 15:05 ` Igor Mammedov
2017-06-29 17:51 ` Roman Kagan
2017-07-07 12:22 ` Igor Mammedov
2017-07-07 12:47 ` Roman Kagan
2017-07-07 13:27 ` Igor Mammedov
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 15/23] hyperv: block SynIC use in QEMU in incompatible configurations Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 16/23] hyperv: make overlay pages for SynIC Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 18/23] hyperv: add synic event flag signaling Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 19/23] hyperv: process SIGNAL_EVENT hypercall Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 20/23] hyperv: process POST_MESSAGE hypercall Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 21/23] hyperv_testdev: add SynIC message and event testmodes Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 22/23] MAINTAINERS: add myself and eyakovlev@ for hyperv* Roman Kagan
2017-06-21 16:24 ` [Qemu-devel] [PATCH v2 23/23] hyperv: update copyright notices Roman Kagan
2017-06-29 15:20 ` [Qemu-devel] [PATCH v2 00/23] hyperv fixes and enhancements Igor Mammedov
2017-06-29 17:58 ` Roman Kagan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.