[PATCHv2 0/4] Add support for some HYPER-V PV features

[PATCHv2 0/4] Add support for some HYPER-V PV features

[Gleb Natapov]

HYPER-V provides PV capabilities for its guests and most new MS Windows
detect and use them automatically. Older Windows guests need additional
drivers to uses PV. This patch series implements some PV capabilities
defined by HYPER-V spec for KVM. Windows guests running on KVM will be
able to take advantage of them.

Changelog:
 v1->v2
  rename kvm_hyperv.h into hyperv.h and move into separate patch
  minor style fixes
  use clear_user-page(0 to zero userspace page 
  use APIC register names when calling kvm_hv_vapic_msr_(read|write)()

Gleb Natapov (4):
  Add HYPE-V header file.
  Implement bare minimum of HYPER-V MSRs.
  Add HYPER-V apic access MSRs.
  Implement NotifyLongSpinWait HYPER-V hypercall.

 arch/x86/include/asm/hyperv.h   |  187 ++++++++++++++++++++++++++++++++
 arch/x86/include/asm/kvm_host.h |    6 +
 arch/x86/include/asm/kvm_para.h |    1 +
 arch/x86/kvm/lapic.c            |   31 ++++++
 arch/x86/kvm/lapic.h            |    8 ++
 arch/x86/kvm/trace.h            |   32 ++++++
 arch/x86/kvm/x86.c              |  227 ++++++++++++++++++++++++++++++++++++++-
 include/linux/kvm.h             |    3 +
 8 files changed, 494 insertions(+), 1 deletions(-)
 create mode 100644 arch/x86/include/asm/hyperv.h

[PATCHv2 1/4] Add HYPE-V header file.

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
---
 arch/x86/include/asm/hyperv.h |  187 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 187 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/include/asm/hyperv.h

diff --git a/arch/x86/include/asm/hyperv.h b/arch/x86/include/asm/hyperv.h
new file mode 100644
index 0000000..91211f3
--- /dev/null
+++ b/arch/x86/include/asm/hyperv.h
@@ -0,0 +1,187 @@
+#ifndef _ASM_X86_KVM_HYPERV_H
+#define _ASM_X86_KVM_HYPERV_H
+
+#include <linux/types.h>
+
+/*
+ * The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent
+ * is set by CPUID(HvCpuIdFunctionVersionAndFeatures).
+ */
+#define HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS	0x40000000
+#define HYPERV_CPUID_INTERFACE			0x40000001
+#define HYPERV_CPUID_VERSION			0x40000002
+#define HYPERV_CPUID_FEATURES			0x40000003
+#define HYPERV_CPUID_ENLIGHTMENT_INFO		0x40000004
+#define HYPERV_CPUID_IMPLEMENT_LIMITS		0x40000005
+
+/*
+ * Feature identification. EAX indicates which features are available
+ * to the partition based upon the current partition privileges.
+ */
+
+/* VP Runtime (HV_X64_MSR_VP_RUNTIME) available */
+#define HV_X64_MSR_VP_RUNTIME_AVAILABLE		(1 << 0)
+/* Partition Reference Counter (HV_X64_MSR_TIME_REF_COUNT) available*/
+#define HV_X64_MSR_TIME_REF_COUNT_AVAILABLE	(1 << 1)
+/*
+ * Basic SynIC MSRs (HV_X64_MSR_SCONTROL through HV_X64_MSR_EOM
+ * and HV_X64_MSR_SINT0 through HV_X64_MSR_SINT15) available
+ */
+#define HV_X64_MSR_SYNIC_AVAILABLE		(1 << 2)
+/*
+ * Synthetic Timer MSRs (HV_X64_MSR_STIMER0_CONFIG through
+ * HV_X64_MSR_STIMER3_COUNT) available
+ */
+#define HV_X64_MSR_SYNTIMER_AVAILABLE		(1 << 3)
+/*
+ * APIC access MSRs (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR)
+ * are available
+ */
+#define HV_X64_MSR_APIC_ACCESS_AVAILABLE	(1 << 4)
+/* Hypercall MSRs (HV_X64_MSR_GUEST_OS_ID and HV_X64_MSR_HYPERCALL) available*/
+#define HV_X64_MSR_HYPERCALL_AVAILABLE		(1 << 5)
+/* Access virtual processor index MSR (HV_X64_MSR_VP_INDEX) available*/
+#define HV_X64_MSR_VP_INDEX_AVAILABLE		(1 << 6)
+/* Virtual system reset MSR (HV_X64_MSR_RESET) is available*/
+#define HV_X64_MSR_RESET_AVAILABLE		(1 << 7)
+ /*
+  * Access statistics pages MSRs (HV_X64_MSR_STATS_PARTITION_RETAIL_PAGE,
+  * HV_X64_MSR_STATS_PARTITION_INTERNAL_PAGE, HV_X64_MSR_STATS_VP_RETAIL_PAGE,
+  * HV_X64_MSR_STATS_VP_INTERNAL_PAGE) available
+  */
+#define HV_X64_MSR_STAT_PAGES_AVAILABLE		(1 << 8)
+
+/*
+ * Feature identification: EBX indicates which flags were specified at
+ * partition creation. The format is the same as the partition creation
+ * flag structure defined in section Partition Creation Flags.
+ */
+#define HV_X64_CREATE_PARTITIONS		(1 << 0)
+#define HV_X64_ACCESS_PARTITION_ID		(1 << 1)
+#define HV_X64_ACCESS_MEMORY_POOL		(1 << 2)
+#define HV_X64_ADJUST_MESSAGE_BUFFERS		(1 << 3)
+#define HV_X64_POST_MESSAGES			(1 << 4)
+#define HV_X64_SIGNAL_EVENTS			(1 << 5)
+#define HV_X64_CREATE_PORT			(1 << 6)
+#define HV_X64_CONNECT_PORT			(1 << 7)
+#define HV_X64_ACCESS_STATS			(1 << 8)
+#define HV_X64_DEBUGGING			(1 << 11)
+#define HV_X64_CPU_POWER_MANAGEMENT		(1 << 12)
+#define HV_X64_CONFIGURE_PROFILER		(1 << 13)
+
+/*
+ * Feature identification. EDX indicates which miscellaneous features
+ * are available to the partition.
+ */
+/* The MWAIT instruction is available (per section MONITOR / MWAIT) */
+#define HV_X64_MWAIT_AVAILABLE				(1 << 0)
+/* Guest debugging support is available */
+#define HV_X64_GUEST_DEBUGGING_AVAILABLE		(1 << 1)
+/* Performance Monitor support is available*/
+#define HV_X64_PERF_MONITOR_AVAILABLE			(1 << 2)
+/* Support for physical CPU dynamic partitioning events is available*/
+#define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE	(1 << 3)
+/*
+ * Support for passing hypercall input parameter block via XMM
+ * registers is available
+ */
+#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE		(1 << 4)
+/* Support for a virtual guest idle state is available */
+#define HV_X64_GUEST_IDLE_STATE_AVAILABLE		(1 << 5)
+
+/*
+ * Implementation recommendations. Indicates which behaviors the hypervisor
+ * recommends the OS implement for optimal performance.
+ */
+ /*
+  * Recommend using hypercall for address space switches rather
+  * than MOV to CR3 instruction
+  */
+#define HV_X64_MWAIT_RECOMMENDED		(1 << 0)
+/* Recommend using hypercall for local TLB flushes rather
+ * than INVLPG or MOV to CR3 instructions */
+#define HV_X64_LOCAL_TLB_FLUSH_RECOMMENDED	(1 << 1)
+/*
+ * Recommend using hypercall for remote TLB flushes rather
+ * than inter-processor interrupts
+ */
+#define HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED	(1 << 2)
+/*
+ * Recommend using MSRs for accessing APIC registers
+ * EOI, ICR and TPR rather than their memory-mapped counterparts
+ */
+#define HV_X64_APIC_ACCESS_RECOMMENDED		(1 << 3)
+/* Recommend using the hypervisor-provided MSR to initiate a system RESET */
+#define HV_X64_SYSTEM_RESET_RECOMMENDED		(1 << 4)
+/*
+ * Recommend using relaxed timing for this partition. If used,
+ * the VM should disable any watchdog timeouts that rely on the
+ * timely delivery of external interrupts
+ */
+#define HV_X64_RELAXED_TIMING_RECOMMENDED	(1 << 5)
+
+/* MSR used to identify the guest OS. */
+#define HV_X64_MSR_GUEST_OS_ID			0x40000000
+
+/* MSR used to setup pages used to communicate with the hypervisor. */
+#define HV_X64_MSR_HYPERCALL			0x40000001
+
+/* MSR used to provide vcpu index */
+#define HV_X64_MSR_VP_INDEX			0x40000002
+
+/* Define the virtual APIC registers */
+#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
+
+/* Define synthetic interrupt controller model specific registers. */
+#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
+
+
+#define HV_X64_MSR_HYPERCALL_ENABLE		0x00000001
+#define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT	12
+#define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_MASK	\
+		(~((1ull << HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT) - 1))
+
+/* Declare the various hypercall operations. */
+#define HV_X64_HV_NOTIFY_LONG_SPIN_WAIT		0x0008
+
+#define HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE		0x00000001
+#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT	12
+#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_MASK	\
+		(~((1ull << HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT) - 1))
+
+#define HV_PROCESSOR_POWER_STATE_C0		0
+#define HV_PROCESSOR_POWER_STATE_C1		1
+#define HV_PROCESSOR_POWER_STATE_C2		2
+#define HV_PROCESSOR_POWER_STATE_C3		3
+
+/* 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
+
+#endif
+
-- 
1.6.5

[PATCHv2 2/4] Implement bare minimum of HYPER-V MSRs.

[Gleb Natapov]

Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
VP_INDEX MSRs.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |    4 +
 arch/x86/include/asm/kvm_para.h |    1 +
 arch/x86/kvm/trace.h            |   32 +++++++
 arch/x86/kvm/x86.c              |  184 ++++++++++++++++++++++++++++++++++++++-
 include/linux/kvm.h             |    1 +
 5 files changed, 221 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 93bee7a..67d19e4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -413,6 +413,10 @@ struct kvm_arch {
 	s64 kvmclock_offset;
 
 	struct kvm_xen_hvm_config xen_hvm_config;
+
+	/* fields used by HYPER-V emulation */
+	u64 hv_guest_os_id;
+	u64 hv_hypercall;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index c584076..ffae142 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -2,6 +2,7 @@
 #define _ASM_X86_KVM_PARA_H
 
 #include <linux/types.h>
+#include <asm/hyperv.h>
 
 /* This CPUID returns the signature 'KVMKVMKVM' in ebx, ecx, and edx.  It
  * should be used to determine that a VM is running under KVM.
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 816e044..1cb3d0e 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -56,6 +56,38 @@ TRACE_EVENT(kvm_hypercall,
 );
 
 /*
+ * Tracepoint for hypercall.
+ */
+TRACE_EVENT(kvm_hv_hypercall,
+	TP_PROTO(__u16 code, bool fast, __u16 rep_cnt, __u16 rep_idx,
+		 __u64 ingpa, __u64 outgpa),
+	TP_ARGS(code, fast, rep_cnt, rep_idx, ingpa, outgpa),
+
+	TP_STRUCT__entry(
+		__field(	__u16, 		code		)
+		__field(	bool,		fast		)
+		__field(	__u16,		rep_cnt		)
+		__field(	__u16,		rep_idx		)
+		__field(	__u64,		ingpa		)
+		__field(	__u64,		outgpa		)
+	),
+
+	TP_fast_assign(
+		__entry->code		= code;
+		__entry->fast		= fast;
+		__entry->rep_cnt	= rep_cnt;
+		__entry->rep_idx	= rep_idx;
+		__entry->ingpa		= ingpa;
+		__entry->outgpa		= outgpa;
+	),
+
+	TP_printk("code 0x%x %s cnt 0x%x idx 0x%x in 0x%llx out 0x%llx",
+		  __entry->code, __entry->fast ? "fast" : "slow",
+		  __entry->rep_cnt, __entry->rep_idx,  __entry->ingpa,
+		  __entry->outgpa)
+);
+
+/*
  * Tracepoint for PIO.
  */
 TRACE_EVENT(kvm_pio,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4d835b6..db0b2b1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -630,7 +630,8 @@ static u32 msrs_to_save[] = {
 #ifdef CONFIG_X86_64
 	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
 #endif
-	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
+	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
+	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
 };
 
 static unsigned num_msrs_to_save;
@@ -1005,6 +1006,69 @@ out:
 	return r;
 }
 
+static bool kvm_hv_hypercall_enabled(struct kvm *kvm)
+{
+	return kvm->arch.hv_hypercall & HV_X64_MSR_HYPERCALL_ENABLE;
+}
+
+static bool kvm_hv_msr_partition_wide(u32 msr)
+{
+	bool r = false;
+	switch (msr) {
+	case HV_X64_MSR_GUEST_OS_ID:
+	case HV_X64_MSR_HYPERCALL:
+		r = true;
+		break;
+	}
+
+	return r;
+}
+
+static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+{
+	struct kvm *kvm = vcpu->kvm;
+
+	switch (msr) {
+	case HV_X64_MSR_GUEST_OS_ID:
+		kvm->arch.hv_guest_os_id = data;
+		/* setting guest os id to zero disables hypercall page */
+		if (!kvm->arch.hv_guest_os_id)
+			kvm->arch.hv_hypercall &= ~HV_X64_MSR_HYPERCALL_ENABLE;
+		break;
+	case HV_X64_MSR_HYPERCALL: {
+		u64 gfn;
+		unsigned long addr;
+		/* if guest os id is not set hypercall should remain disabled */
+		if (!kvm->arch.hv_guest_os_id && data)
+			break;
+		kvm->arch.hv_hypercall = data;
+		if (!kvm_hv_hypercall_enabled(kvm))
+			break;
+		gfn = kvm->arch.hv_hypercall >>
+			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr))
+			return 1;
+		kvm_x86_ops->patch_hypercall(vcpu, (unsigned char *)addr);
+		((unsigned char *)addr)[3] = 0xc3; /* ret */
+		break;
+	}
+	default:
+		pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x "
+			  "data 0x%llx\n", msr, data);
+		return 1;
+	}
+	return 0;
+}
+
+static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+{
+	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
+		  msr, data);
+
+	return 1;
+}
+
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {
 	switch (msr) {
@@ -1119,6 +1183,16 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		pr_unimpl(vcpu, "unimplemented perfctr wrmsr: "
 			"0x%x data 0x%llx\n", msr, data);
 		break;
+	case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
+		if (kvm_hv_msr_partition_wide(msr)) {
+			int r;
+			mutex_lock(&vcpu->kvm->lock);
+			r = set_msr_hyperv_pw(vcpu, msr, data);
+			mutex_unlock(&vcpu->kvm->lock);
+			return r;
+		} else
+			return set_msr_hyperv(vcpu, msr, data);
+		break;
 	default:
 		if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr))
 			return xen_hvm_config(vcpu, data);
@@ -1218,6 +1292,48 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	return 0;
 }
 
+static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
+{
+	u64 data = 0;
+	struct kvm *kvm = vcpu->kvm;
+
+	switch (msr) {
+	case HV_X64_MSR_GUEST_OS_ID:
+		data = kvm->arch.hv_guest_os_id;
+		break;
+	case HV_X64_MSR_HYPERCALL:
+		data = kvm->arch.hv_hypercall;
+		break;
+	default:
+		pr_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
+		return 1;
+	}
+
+	*pdata = data;
+	return 0;
+}
+
+static int get_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
+{
+	u64 data = 0;
+
+	switch (msr) {
+	case HV_X64_MSR_VP_INDEX: {
+		int r;
+		struct kvm_vcpu *v;
+		kvm_for_each_vcpu(r, v, vcpu->kvm)
+			if (v == vcpu)
+				data = r;
+		break;
+	}
+	default:
+		pr_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
+		return 1;
+	}
+	*pdata = data;
+	return 0;
+}
+
 int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 {
 	u64 data;
@@ -1284,6 +1400,16 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_IA32_MCG_STATUS:
 	case MSR_IA32_MC0_CTL ... MSR_IA32_MC0_CTL + 4 * KVM_MAX_MCE_BANKS - 1:
 		return get_msr_mce(vcpu, msr, pdata);
+	case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
+		if (kvm_hv_msr_partition_wide(msr)) {
+			int r;
+			mutex_lock(&vcpu->kvm->lock);
+			r = get_msr_hyperv_pw(vcpu, msr, pdata);
+			mutex_unlock(&vcpu->kvm->lock);
+			return r;
+		} else
+			return get_msr_hyperv(vcpu, msr, pdata);
+		break;
 	default:
 		if (!ignore_msrs) {
 			pr_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr);
@@ -1399,6 +1525,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_XEN_HVM:
 	case KVM_CAP_ADJUST_CLOCK:
 	case KVM_CAP_VCPU_EVENTS:
+	case KVM_CAP_HYPERV:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -3619,11 +3746,66 @@ static inline gpa_t hc_gpa(struct kvm_vcpu *vcpu, unsigned long a0,
 		return a0 | ((gpa_t)a1 << 32);
 }
 
+int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
+{
+	u64 param, ingpa, outgpa, ret;
+	uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
+	bool fast, longmode;
+	int cs_db, cs_l;
+
+	/*
+	 * hypercall generates UD from non zero cpl and real mode
+	 * per HYPER-V spec
+	 */
+	if (kvm_x86_ops->get_cpl(vcpu) != 0 || !(vcpu->arch.cr0 & X86_CR0_PE)) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 0;
+	}
+
+	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
+	longmode = is_long_mode(vcpu) && cs_l == 1;
+
+	if (longmode) {
+		param = kvm_register_read(vcpu, VCPU_REGS_RCX);
+		ingpa = kvm_register_read(vcpu, VCPU_REGS_RDX);
+		outgpa = kvm_register_read(vcpu, VCPU_REGS_R8);
+	} else {
+		param = (kvm_register_read(vcpu, VCPU_REGS_RDX) << 32) |
+			(kvm_register_read(vcpu, VCPU_REGS_RAX) & 0xffffff);
+		ingpa = (kvm_register_read(vcpu, VCPU_REGS_RBX) << 32) |
+			(kvm_register_read(vcpu, VCPU_REGS_RCX) & 0xffffff);
+		outgpa = (kvm_register_read(vcpu, VCPU_REGS_RDI) << 32) |
+			(kvm_register_read(vcpu, VCPU_REGS_RSI) & 0xffffff);
+	}
+
+	code = param & 0xffff;
+	fast = (param >> 16) & 0x1;
+	rep_cnt = (param >> 32) & 0xfff;
+	rep_idx = (param >> 48) & 0xfff;
+
+	trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
+
+	res = HV_STATUS_INVALID_HYPERCALL_CODE;
+
+	ret = res | (((u64)rep_done & 0xfff) << 32);
+	if (longmode) {
+		kvm_register_write(vcpu, VCPU_REGS_RAX, ret);
+	} else {
+		kvm_register_write(vcpu, VCPU_REGS_RDX, ret >> 32);
+		kvm_register_write(vcpu, VCPU_REGS_RAX, ret & 0xffffffff);
+	}
+
+	return 1;
+}
+
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
 	unsigned long nr, a0, a1, a2, a3, ret;
 	int r = 1;
 
+	if (kvm_hv_hypercall_enabled(vcpu->kvm))
+		return kvm_hv_hypercall(vcpu);
+
 	nr = kvm_register_read(vcpu, VCPU_REGS_RAX);
 	a0 = kvm_register_read(vcpu, VCPU_REGS_RBX);
 	a1 = kvm_register_read(vcpu, VCPU_REGS_RCX);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index f2feef6..e227cba 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -497,6 +497,7 @@ struct kvm_ioeventfd {
 #endif
 #define KVM_CAP_S390_PSW 42
 #define KVM_CAP_PPC_SEGSTATE 43
+#define KVM_CAP_HYPERV 44
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.6.5

[PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |    2 +
 arch/x86/kvm/lapic.c            |   31 +++++++++++++++++++++++++++++
 arch/x86/kvm/lapic.h            |    8 +++++++
 arch/x86/kvm/x86.c              |   41 ++++++++++++++++++++++++++++++++++++--
 include/linux/kvm.h             |    1 +
 5 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 67d19e4..a1f0b5d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -363,6 +363,8 @@ struct kvm_vcpu_arch {
 	/* used for guest single stepping over the given code position */
 	u16 singlestep_cs;
 	unsigned long singlestep_rip;
+	/* fields used by HYPER-V emulation */
+	u64 hv_vapic;
 };
 
 struct kvm_mem_alias {
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ba8c045..4b224f9 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1246,3 +1246,34 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 
 	return 0;
 }
+
+int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!irqchip_in_kernel(vcpu->kvm))
+		return 1;
+
+	/* if this is ICR write vector before command */
+	if (reg == APIC_ICR)
+		apic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
+	return apic_reg_write(apic, reg, (u32)data);
+}
+
+int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	u32 low, high = 0;
+
+	if (!irqchip_in_kernel(vcpu->kvm))
+		return 1;
+
+	if (apic_reg_read(apic, reg, 4, &low))
+		return 1;
+	if (reg == APIC_ICR)
+		apic_reg_read(apic, APIC_ICR2, 4, &high);
+
+	*data = (((u64)high) << 32) | low;
+
+	return 0;
+}
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 40010b0..d081cb6 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -48,4 +48,12 @@ void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu);
 
 int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
 int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
+
+int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
+int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
+
+static inline bool kvm_hv_vapic_enabled(struct kvm_vcpu *vcpu)
+{
+	return !!(vcpu->arch.hv_vapic & HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE);
+}
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index db0b2b1..2fe555c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -632,6 +632,7 @@ static u32 msrs_to_save[] = {
 #endif
 	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
 	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
+	HV_X64_MSR_APIC_ASSIST_PAGE,
 };
 
 static unsigned num_msrs_to_save;
@@ -1063,10 +1064,37 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 
 static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {
-	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
-		  msr, data);
+	switch (msr) {
+	case HV_X64_MSR_APIC_ASSIST_PAGE: {
+		unsigned long vaddr;
+		void *addr;
+		struct page *page;
+		vcpu->arch.hv_vapic = data;
+		if (!kvm_hv_vapic_enabled(vcpu))
+			break;
+		vaddr = gfn_to_hva(vcpu->kvm, data >>
+				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
+		if (kvm_is_error_hva(vaddr))
+			return 1;
+		page = virt_to_page(vaddr);
+		addr = kmap_atomic(page, KM_USER0);
+		clear_user_page(addr, vaddr, page);
+		kunmap_atomic(addr, KM_USER0);
+		break;
+	}
+	case HV_X64_MSR_EOI:
+		return kvm_hv_vapic_msr_write(vcpu, APIC_EOI, data);
+	case HV_X64_MSR_ICR:
+		return kvm_hv_vapic_msr_write(vcpu, APIC_ICR, data);
+	case HV_X64_MSR_TPR:
+		return kvm_hv_vapic_msr_write(vcpu, APIC_TASKPRI, data);
+	default:
+		pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x "
+			  "data 0x%llx\n", msr, data);
+		return 1;
+	}
 
-	return 1;
+	return 0;
 }
 
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
@@ -1326,6 +1354,12 @@ static int get_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 				data = r;
 		break;
 	}
+	case HV_X64_MSR_EOI:
+		return kvm_hv_vapic_msr_read(vcpu, APIC_EOI, pdata);
+	case HV_X64_MSR_ICR:
+		return kvm_hv_vapic_msr_read(vcpu, APIC_ICR, pdata);
+	case HV_X64_MSR_TPR:
+		return kvm_hv_vapic_msr_read(vcpu, APIC_TASKPRI, pdata);
 	default:
 		pr_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
 		return 1;
@@ -1526,6 +1560,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_ADJUST_CLOCK:
 	case KVM_CAP_VCPU_EVENTS:
 	case KVM_CAP_HYPERV:
+	case KVM_CAP_HYPERV_VAPIC:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index e227cba..5ce6173 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -498,6 +498,7 @@ struct kvm_ioeventfd {
 #define KVM_CAP_S390_PSW 42
 #define KVM_CAP_PPC_SEGSTATE 43
 #define KVM_CAP_HYPERV 44
+#define KVM_CAP_HYPERV_VAPIC 45
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.6.5

[PATCHv2 4/4] Implement NotifyLongSpinWait HYPER-V hypercall.

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
---
 arch/x86/kvm/x86.c  |   10 +++++++++-
 include/linux/kvm.h |    1 +
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2fe555c..0421592 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1561,6 +1561,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_VCPU_EVENTS:
 	case KVM_CAP_HYPERV:
 	case KVM_CAP_HYPERV_VAPIC:
+	case KVM_CAP_HYPERV_SPIN:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -3820,7 +3821,14 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
 
 	trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
 
-	res = HV_STATUS_INVALID_HYPERCALL_CODE;
+	switch (code) {
+	case HV_X64_HV_NOTIFY_LONG_SPIN_WAIT:
+		kvm_vcpu_on_spin(vcpu);
+		break;
+	default:
+		res = HV_STATUS_INVALID_HYPERCALL_CODE;
+		break;
+	}
 
 	ret = res | (((u64)rep_done & 0xfff) << 32);
 	if (longmode) {
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 5ce6173..4c4937e 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -499,6 +499,7 @@ struct kvm_ioeventfd {
 #define KVM_CAP_PPC_SEGSTATE 43
 #define KVM_CAP_HYPERV 44
 #define KVM_CAP_HYPERV_VAPIC 45
+#define KVM_CAP_HYPERV_SPIN 46
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.6.5

Re: [PATCHv2 2/4] Implement bare minimum of HYPER-V MSRs.

[Avi Kivity]

On 01/17/2010 11:03 AM, Gleb Natapov wrote:
> Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
> VP_INDEX MSRs.
>
>
>   TRACE_EVENT(kvm_pio,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4d835b6..db0b2b1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -630,7 +630,8 @@ static u32 msrs_to_save[] = {
>   #ifdef CONFIG_X86_64
>   	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>   #endif
> -	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> +	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> +	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
>   };
>    

These will be disabled since the msrs don't exist on the host.  See the 
comment above and KVM_SAVE_MSRS_BEGIN.

> +	case HV_X64_MSR_HYPERCALL: {
> +		u64 gfn;
> +		unsigned long addr;
> +		/* if guest os id is not set hypercall should remain disabled */
> +		if (!kvm->arch.hv_guest_os_id&&  data)
> +			break;
> +		kvm->arch.hv_hypercall = data;
> +		if (!kvm_hv_hypercall_enabled(kvm))
> +			break;
> +		gfn = kvm->arch.hv_hypercall>>
> +			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
> +		addr = gfn_to_hva(kvm, gfn);
> +		if (kvm_is_error_hva(addr))
> +			return 1;
>    

Should di the error check before assigning, perhaps.

> +		kvm_x86_ops->patch_hypercall(vcpu, (unsigned char *)addr);
> +		((unsigned char *)addr)[3] = 0xc3; /* ret */
>    

kvm_write_guest(), this can fault.

> +int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> +{
> +	u64 param, ingpa, outgpa, ret;
> +	uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
> +	bool fast, longmode;
> +	int cs_db, cs_l;
> +
> +	/*
> +	 * hypercall generates UD from non zero cpl and real mode
> +	 * per HYPER-V spec
> +	 */
> +	if (kvm_x86_ops->get_cpl(vcpu) != 0 || !(vcpu->arch.cr0&  X86_CR0_PE)) {
>    

Use kvm_read_cr0_bits() to avoid caching.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Avi Kivity]

On 01/17/2010 11:03 AM, Gleb Natapov wrote:

> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> Signed-off-by: Vadim Rozenfeld<vrozenfe@redhat.com>
>    

Changelog entry.

>
>   struct kvm_mem_alias {
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index ba8c045..4b224f9 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1246,3 +1246,34 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
>
>   	return 0;
>   }
> +
> +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		return 1;
> +
> +	/* if this is ICR write vector before command */
> +	if (reg == APIC_ICR)
> +		apic_reg_write(apic, APIC_ICR2, (u32)(data>>  32));
> +	return apic_reg_write(apic, reg, (u32)data);
> +}
> +
> +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +	u32 low, high = 0;
> +
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		return 1;
> +
> +	if (apic_reg_read(apic, reg, 4,&low))
> +		return 1;
> +	if (reg == APIC_ICR)
> +		apic_reg_read(apic, APIC_ICR2, 4,&high);
> +
> +	*data = (((u64)high)<<  32) | low;
> +
> +	return 0;
> +}
>    

I prefer putting this in x86.c (maybe later split into hyperv.c).

> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 40010b0..d081cb6 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -48,4 +48,12 @@ void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu);
>
>   int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
>   int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> +
> +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
> +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> +
> +static inline bool kvm_hv_vapic_enabled(struct kvm_vcpu *vcpu)
> +{
> +	return !!(vcpu->arch.hv_vapic&  HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE);
> +}
>    

Are you sure that vapic enabled is equivalent to apic assist page enable?

!! not required.

>   #endif
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index db0b2b1..2fe555c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -632,6 +632,7 @@ static u32 msrs_to_save[] = {
>   #endif
>   	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>   	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
> +	HV_X64_MSR_APIC_ASSIST_PAGE,
>   };
>
>    

Will be dropped by msr validation.

>   static unsigned num_msrs_to_save;
> @@ -1063,10 +1064,37 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>
>   static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   {
> -	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
> -		  msr, data);
> +	switch (msr) {
> +	case HV_X64_MSR_APIC_ASSIST_PAGE: {
> +		unsigned long vaddr;
> +		void *addr;
> +		struct page *page;
> +		vcpu->arch.hv_vapic = data;
> +		if (!kvm_hv_vapic_enabled(vcpu))
> +			break;
> +		vaddr = gfn_to_hva(vcpu->kvm, data>>
> +				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
> +		if (kvm_is_error_hva(vaddr))
> +			return 1;
> +		page = virt_to_page(vaddr);
>    

virt_to_page() takes a kernel address, not a user address.  This is 
get_user_pages().  But I think the whole thing is done better with 
put_user().

> +		addr = kmap_atomic(page, KM_USER0);
> +		clear_user_page(addr, vaddr, page);
> +		kunmap_atomic(addr, KM_USER0);
>    

Surprising that clear_user_page needs kmap_atomic() (but true).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCHv2 4/4] Implement NotifyLongSpinWait HYPER-V hypercall.

[Avi Kivity]

On 01/17/2010 11:03 AM, Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> Signed-off-by: Vadim Rozenfeld<vrozenfe@redhat.com>
>    

Changelog entry.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Christoph Hellwig]

On Sun, Jan 17, 2010 at 02:20:32PM +0200, Avi Kivity wrote:
>> +		addr = kmap_atomic(page, KM_USER0);
>> +		clear_user_page(addr, vaddr, page);
>> +		kunmap_atomic(addr, KM_USER0);
>>    
>
> Surprising that clear_user_page needs kmap_atomic() (but true).

There's a clear_user_highpage helper to take care of it for you.

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Vadim Rozenfeld]

On Sun, 2010-01-17 at 14:20 +0200, Avi Kivity wrote:
> On 01/17/2010 11:03 AM, Gleb Natapov wrote:
> 
> > Signed-off-by: Gleb Natapov<gleb@redhat.com>
> > Signed-off-by: Vadim Rozenfeld<vrozenfe@redhat.com>
> >    
> 
> Changelog entry.
> 
> >
> >   struct kvm_mem_alias {
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index ba8c045..4b224f9 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1246,3 +1246,34 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
> >
> >   	return 0;
> >   }
> > +
> > +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
> > +{
> > +	struct kvm_lapic *apic = vcpu->arch.apic;
> > +
> > +	if (!irqchip_in_kernel(vcpu->kvm))
> > +		return 1;
> > +
> > +	/* if this is ICR write vector before command */
> > +	if (reg == APIC_ICR)
> > +		apic_reg_write(apic, APIC_ICR2, (u32)(data>>  32));
> > +	return apic_reg_write(apic, reg, (u32)data);
> > +}
> > +
> > +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
> > +{
> > +	struct kvm_lapic *apic = vcpu->arch.apic;
> > +	u32 low, high = 0;
> > +
> > +	if (!irqchip_in_kernel(vcpu->kvm))
> > +		return 1;
> > +
> > +	if (apic_reg_read(apic, reg, 4,&low))
> > +		return 1;
> > +	if (reg == APIC_ICR)
> > +		apic_reg_read(apic, APIC_ICR2, 4,&high);
> > +
> > +	*data = (((u64)high)<<  32) | low;
> > +
> > +	return 0;
> > +}
> >    
> 
> I prefer putting this in x86.c (maybe later split into hyperv.c).
> 
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index 40010b0..d081cb6 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -48,4 +48,12 @@ void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu);
> >
> >   int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
> >   int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> > +
> > +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
> > +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> > +
> > +static inline bool kvm_hv_vapic_enabled(struct kvm_vcpu *vcpu)
> > +{
> > +	return !!(vcpu->arch.hv_vapic&  HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE);
> > +}
> >    
> 
> Are you sure that vapic enabled is equivalent to apic assist page enable?
At least, when it is disable, the EOI interception mechanism won't
work.   
> 
> !! not required.
> 
> >   #endif
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index db0b2b1..2fe555c 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -632,6 +632,7 @@ static u32 msrs_to_save[] = {
> >   #endif
> >   	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> >   	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
> > +	HV_X64_MSR_APIC_ASSIST_PAGE,
> >   };
> >
> >    
> 
> Will be dropped by msr validation.
> 
> >   static unsigned num_msrs_to_save;
> > @@ -1063,10 +1064,37 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >
> >   static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >   {
> > -	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
> > -		  msr, data);
> > +	switch (msr) {
> > +	case HV_X64_MSR_APIC_ASSIST_PAGE: {
> > +		unsigned long vaddr;
> > +		void *addr;
> > +		struct page *page;
> > +		vcpu->arch.hv_vapic = data;
> > +		if (!kvm_hv_vapic_enabled(vcpu))
> > +			break;
> > +		vaddr = gfn_to_hva(vcpu->kvm, data>>
> > +				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
> > +		if (kvm_is_error_hva(vaddr))
> > +			return 1;
> > +		page = virt_to_page(vaddr);
> >    
> 
> virt_to_page() takes a kernel address, not a user address.  This is 
> get_user_pages().  But I think the whole thing is done better with 
> put_user().
> 
> > +		addr = kmap_atomic(page, KM_USER0);
> > +		clear_user_page(addr, vaddr, page);
> > +		kunmap_atomic(addr, KM_USER0);
> >    
> 
> Surprising that clear_user_page needs kmap_atomic() (but true).
> 

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Gleb Natapov]

On Sun, Jan 17, 2010 at 02:20:32PM +0200, Avi Kivity wrote:
> On 01/17/2010 11:03 AM, Gleb Natapov wrote:
> 
> >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >Signed-off-by: Vadim Rozenfeld<vrozenfe@redhat.com>
> 
> Changelog entry.
> 
> >
> >  struct kvm_mem_alias {
> >diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >index ba8c045..4b224f9 100644
> >--- a/arch/x86/kvm/lapic.c
> >+++ b/arch/x86/kvm/lapic.c
> >@@ -1246,3 +1246,34 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
> >
> >  	return 0;
> >  }
> >+
> >+int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
> >+{
> >+	struct kvm_lapic *apic = vcpu->arch.apic;
> >+
> >+	if (!irqchip_in_kernel(vcpu->kvm))
> >+		return 1;
> >+
> >+	/* if this is ICR write vector before command */
> >+	if (reg == APIC_ICR)
> >+		apic_reg_write(apic, APIC_ICR2, (u32)(data>>  32));
> >+	return apic_reg_write(apic, reg, (u32)data);
> >+}
> >+
> >+int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
> >+{
> >+	struct kvm_lapic *apic = vcpu->arch.apic;
> >+	u32 low, high = 0;
> >+
> >+	if (!irqchip_in_kernel(vcpu->kvm))
> >+		return 1;
> >+
> >+	if (apic_reg_read(apic, reg, 4,&low))
> >+		return 1;
> >+	if (reg == APIC_ICR)
> >+		apic_reg_read(apic, APIC_ICR2, 4,&high);
> >+
> >+	*data = (((u64)high)<<  32) | low;
> >+
> >+	return 0;
> >+}
> 
> I prefer putting this in x86.c (maybe later split into hyperv.c).
> 
This implements part of apic behaviour. It uses internal lapic functions
like apic_reg_read()/apic_reg_write(). Why move it from lapic.c?

> >diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >index 40010b0..d081cb6 100644
> >--- a/arch/x86/kvm/lapic.h
> >+++ b/arch/x86/kvm/lapic.h
> >@@ -48,4 +48,12 @@ void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu);
> >
> >  int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
> >  int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> >+
> >+int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
> >+int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> >+
> >+static inline bool kvm_hv_vapic_enabled(struct kvm_vcpu *vcpu)
> >+{
> >+	return !!(vcpu->arch.hv_vapic&  HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE);
> >+}
> 
> Are you sure that vapic enabled is equivalent to apic assist page enable?
> 
> !! not required.
> 
The function is not used to check if vapic is enabled, so the name
should be changed.

> >  #endif
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index db0b2b1..2fe555c 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -632,6 +632,7 @@ static u32 msrs_to_save[] = {
> >  #endif
> >  	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> >  	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
> >+	HV_X64_MSR_APIC_ASSIST_PAGE,
> >  };
> >
> 
> Will be dropped by msr validation.
> 
> >  static unsigned num_msrs_to_save;
> >@@ -1063,10 +1064,37 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >
> >  static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >  {
> >-	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
> >-		  msr, data);
> >+	switch (msr) {
> >+	case HV_X64_MSR_APIC_ASSIST_PAGE: {
> >+		unsigned long vaddr;
> >+		void *addr;
> >+		struct page *page;
> >+		vcpu->arch.hv_vapic = data;
> >+		if (!kvm_hv_vapic_enabled(vcpu))
> >+			break;
> >+		vaddr = gfn_to_hva(vcpu->kvm, data>>
> >+				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
> >+		if (kvm_is_error_hva(vaddr))
> >+			return 1;
> >+		page = virt_to_page(vaddr);
> 
> virt_to_page() takes a kernel address, not a user address.  This is
> get_user_pages().  But I think the whole thing is done better with
> put_user().
> 
So there is no function to get struct page from user virtual address?

> >+		addr = kmap_atomic(page, KM_USER0);
> >+		clear_user_page(addr, vaddr, page);
> >+		kunmap_atomic(addr, KM_USER0);
> 
> Surprising that clear_user_page needs kmap_atomic() (but true).
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Gleb Natapov]

On Sun, Jan 17, 2010 at 07:32:32AM -0500, Christoph Hellwig wrote:
> On Sun, Jan 17, 2010 at 02:20:32PM +0200, Avi Kivity wrote:
> >> +		addr = kmap_atomic(page, KM_USER0);
> >> +		clear_user_page(addr, vaddr, page);
> >> +		kunmap_atomic(addr, KM_USER0);
> >>    
> >
> > Surprising that clear_user_page needs kmap_atomic() (but true).
> 
> There's a clear_user_highpage helper to take care of it for you.
I copied code from the instead of using helper faction for
some unknown to me reason. Anyway if I can't get struct page from
user virtual address I can't use it. Actually I am not sure the page
should be zeroed at all. Spec only descries first dword of the page and
doesn't require zeroing the reset as far as I see.

--
			Gleb.

Re: [PATCHv2 2/4] Implement bare minimum of HYPER-V MSRs.

[Gleb Natapov]

On Sun, Jan 17, 2010 at 02:10:45PM +0200, Avi Kivity wrote:
> On 01/17/2010 11:03 AM, Gleb Natapov wrote:
> >Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
> >VP_INDEX MSRs.
> >
> >
> >  TRACE_EVENT(kvm_pio,
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 4d835b6..db0b2b1 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -630,7 +630,8 @@ static u32 msrs_to_save[] = {
> >  #ifdef CONFIG_X86_64
> >  	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
> >  #endif
> >-	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> >+	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> >+	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
> >  };
> 
> These will be disabled since the msrs don't exist on the host.  See
> the comment above and KVM_SAVE_MSRS_BEGIN.
> 
I see. Why not have two arrays?

> >+	case HV_X64_MSR_HYPERCALL: {
> >+		u64 gfn;
> >+		unsigned long addr;
> >+		/* if guest os id is not set hypercall should remain disabled */
> >+		if (!kvm->arch.hv_guest_os_id&&  data)
> >+			break;
> >+		kvm->arch.hv_hypercall = data;
> >+		if (!kvm_hv_hypercall_enabled(kvm))
> >+			break;
> >+		gfn = kvm->arch.hv_hypercall>>
> >+			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
> >+		addr = gfn_to_hva(kvm, gfn);
> >+		if (kvm_is_error_hva(addr))
> >+			return 1;
> 
> Should di the error check before assigning, perhaps.
> 
Spec doesn't tell. And guest will get #GP and BSOD anyway.

> >+		kvm_x86_ops->patch_hypercall(vcpu, (unsigned char *)addr);
> >+		((unsigned char *)addr)[3] = 0xc3; /* ret */
> 
> kvm_write_guest(), this can fault.
> 
> >+int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> >+{
> >+	u64 param, ingpa, outgpa, ret;
> >+	uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
> >+	bool fast, longmode;
> >+	int cs_db, cs_l;
> >+
> >+	/*
> >+	 * hypercall generates UD from non zero cpl and real mode
> >+	 * per HYPER-V spec
> >+	 */
> >+	if (kvm_x86_ops->get_cpl(vcpu) != 0 || !(vcpu->arch.cr0&  X86_CR0_PE)) {
> 
> Use kvm_read_cr0_bits() to avoid caching.
> 
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Christoph Hellwig]

On Sun, Jan 17, 2010 at 02:41:42PM +0200, Gleb Natapov wrote:
> I copied code from the instead of using helper faction for
> some unknown to me reason. Anyway if I can't get struct page from
> user virtual address I can't use it. Actually I am not sure the page
> should be zeroed at all. Spec only descries first dword of the page and
> doesn't require zeroing the reset as far as I see.

There's a clear_user() function that just takes a user virtual address
and a size.

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Avi Kivity]

On 01/17/2010 02:36 PM, Gleb Natapov wrote:
> +
>>> +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
>>> +{
>>> +	struct kvm_lapic *apic = vcpu->arch.apic;
>>> +	u32 low, high = 0;
>>> +
>>> +	if (!irqchip_in_kernel(vcpu->kvm))
>>> +		return 1;
>>> +
>>> +	if (apic_reg_read(apic, reg, 4,&low))
>>> +		return 1;
>>> +	if (reg == APIC_ICR)
>>> +		apic_reg_read(apic, APIC_ICR2, 4,&high);
>>> +
>>> +	*data = (((u64)high)<<   32) | low;
>>> +
>>> +	return 0;
>>> +}
>>>        
>> I prefer putting this in x86.c (maybe later split into hyperv.c).
>>
>>      
> This implements part of apic behaviour. It uses internal lapic functions
> like apic_reg_read()/apic_reg_write(). Why move it from lapic.c?
>    

The new functions implement hyper-v behaviour.  Why scatter them all around?

Maybe apic_reg_{read,write} need to be exported.

>>>   static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>>   {
>>> -	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
>>> -		  msr, data);
>>> +	switch (msr) {
>>> +	case HV_X64_MSR_APIC_ASSIST_PAGE: {
>>> +		unsigned long vaddr;
>>> +		void *addr;
>>> +		struct page *page;
>>> +		vcpu->arch.hv_vapic = data;
>>> +		if (!kvm_hv_vapic_enabled(vcpu))
>>> +			break;
>>> +		vaddr = gfn_to_hva(vcpu->kvm, data>>
>>> +				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
>>> +		if (kvm_is_error_hva(vaddr))
>>> +			return 1;
>>> +		page = virt_to_page(vaddr);
>>>        
>> virt_to_page() takes a kernel address, not a user address.  This is
>> get_user_pages().  But I think the whole thing is done better with
>> put_user().
>>
>>      
> So there is no function to get struct page from user virtual address?
>    

get_user_pages_fast().


-- 
error compiling committee.c: too many arguments to function

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Gleb Natapov]

On Sun, Jan 17, 2010 at 07:44:57AM -0500, Christoph Hellwig wrote:
> On Sun, Jan 17, 2010 at 02:41:42PM +0200, Gleb Natapov wrote:
> > I copied code from the instead of using helper faction for
> > some unknown to me reason. Anyway if I can't get struct page from
> > user virtual address I can't use it. Actually I am not sure the page
> > should be zeroed at all. Spec only descries first dword of the page and
> > doesn't require zeroing the reset as far as I see.
> 
> There's a clear_user() function that just takes a user virtual address
> and a size.
Precisely what I need. Thanks!

--
			Gleb.

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Avi Kivity]

On 01/17/2010 02:46 PM, Gleb Natapov wrote:
> On Sun, Jan 17, 2010 at 07:44:57AM -0500, Christoph Hellwig wrote:
>    
>> On Sun, Jan 17, 2010 at 02:41:42PM +0200, Gleb Natapov wrote:
>>      
>>> I copied code from the instead of using helper faction for
>>> some unknown to me reason. Anyway if I can't get struct page from
>>> user virtual address I can't use it. Actually I am not sure the page
>>> should be zeroed at all. Spec only descries first dword of the page and
>>> doesn't require zeroing the reset as far as I see.
>>>        
>> There's a clear_user() function that just takes a user virtual address
>> and a size.
>>      
> Precisely what I need. Thanks!
>
>    

But clear_user_page() is confusingly named.  I assume it deals with 
virtually tagged caches on archs that have them?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCHv2 2/4] Implement bare minimum of HYPER-V MSRs.

[Avi Kivity]

On 01/17/2010 02:44 PM, Gleb Natapov wrote:
> On Sun, Jan 17, 2010 at 02:10:45PM +0200, Avi Kivity wrote:
>    
>> On 01/17/2010 11:03 AM, Gleb Natapov wrote:
>>      
>>> Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
>>> VP_INDEX MSRs.
>>>
>>>
>>>   TRACE_EVENT(kvm_pio,
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 4d835b6..db0b2b1 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -630,7 +630,8 @@ static u32 msrs_to_save[] = {
>>>   #ifdef CONFIG_X86_64
>>>   	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>>>   #endif
>>> -	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
>>> +	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>>> +	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
>>>   };
>>>        
>> These will be disabled since the msrs don't exist on the host.  See
>> the comment above and KVM_SAVE_MSRS_BEGIN.
>>
>>      
> I see. Why not have two arrays?
>    

Clearly better.

>>> +	case HV_X64_MSR_HYPERCALL: {
>>> +		u64 gfn;
>>> +		unsigned long addr;
>>> +		/* if guest os id is not set hypercall should remain disabled */
>>> +		if (!kvm->arch.hv_guest_os_id&&   data)
>>> +			break;
>>> +		kvm->arch.hv_hypercall = data;
>>> +		if (!kvm_hv_hypercall_enabled(kvm))
>>> +			break;
>>> +		gfn = kvm->arch.hv_hypercall>>
>>> +			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
>>> +		addr = gfn_to_hva(kvm, gfn);
>>> +		if (kvm_is_error_hva(addr))
>>> +			return 1;
>>>        
>> Should di the error check before assigning, perhaps.
>>
>>      
> Spec doesn't tell. And guest will get #GP and BSOD anyway.
>    

Well, all msrs I know of either #GP, or store the value and do what 
they're supposed to do, never both.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Gleb Natapov]

On Sun, Jan 17, 2010 at 02:46:46PM +0200, Avi Kivity wrote:
> On 01/17/2010 02:36 PM, Gleb Natapov wrote:
> >+
> >>>+int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
> >>>+{
> >>>+	struct kvm_lapic *apic = vcpu->arch.apic;
> >>>+	u32 low, high = 0;
> >>>+
> >>>+	if (!irqchip_in_kernel(vcpu->kvm))
> >>>+		return 1;
> >>>+
> >>>+	if (apic_reg_read(apic, reg, 4,&low))
> >>>+		return 1;
> >>>+	if (reg == APIC_ICR)
> >>>+		apic_reg_read(apic, APIC_ICR2, 4,&high);
> >>>+
> >>>+	*data = (((u64)high)<<   32) | low;
> >>>+
> >>>+	return 0;
> >>>+}
> >>I prefer putting this in x86.c (maybe later split into hyperv.c).
> >>
> >This implements part of apic behaviour. It uses internal lapic functions
> >like apic_reg_read()/apic_reg_write(). Why move it from lapic.c?
> 
> The new functions implement hyper-v behaviour.  Why scatter them all around?
> 
Each hyper-v extension is pretty much independent one from another, so
why not group things by functionality instead. All apic related code in
lapic.c.

> Maybe apic_reg_{read,write} need to be exported.
> 
This is really internal API. It doesn't even check if apic is created.

> >>>  static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >>>  {
> >>>-	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
> >>>-		  msr, data);
> >>>+	switch (msr) {
> >>>+	case HV_X64_MSR_APIC_ASSIST_PAGE: {
> >>>+		unsigned long vaddr;
> >>>+		void *addr;
> >>>+		struct page *page;
> >>>+		vcpu->arch.hv_vapic = data;
> >>>+		if (!kvm_hv_vapic_enabled(vcpu))
> >>>+			break;
> >>>+		vaddr = gfn_to_hva(vcpu->kvm, data>>
> >>>+				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
> >>>+		if (kvm_is_error_hva(vaddr))
> >>>+			return 1;
> >>>+		page = virt_to_page(vaddr);
> >>virt_to_page() takes a kernel address, not a user address.  This is
> >>get_user_pages().  But I think the whole thing is done better with
> >>put_user().
> >>
> >So there is no function to get struct page from user virtual address?
> 
> get_user_pages_fast().
> 
Doh.

--
			Gleb.

Re: [PATCHv2 2/4] Implement bare minimum of HYPER-V MSRs.

[Gleb Natapov]

On Sun, Jan 17, 2010 at 02:49:30PM +0200, Avi Kivity wrote:
> On 01/17/2010 02:44 PM, Gleb Natapov wrote:
> >On Sun, Jan 17, 2010 at 02:10:45PM +0200, Avi Kivity wrote:
> >>On 01/17/2010 11:03 AM, Gleb Natapov wrote:
> >>>Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
> >>>VP_INDEX MSRs.
> >>>
> >>>
> >>>  TRACE_EVENT(kvm_pio,
> >>>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >>>index 4d835b6..db0b2b1 100644
> >>>--- a/arch/x86/kvm/x86.c
> >>>+++ b/arch/x86/kvm/x86.c
> >>>@@ -630,7 +630,8 @@ static u32 msrs_to_save[] = {
> >>>  #ifdef CONFIG_X86_64
> >>>  	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
> >>>  #endif
> >>>-	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> >>>+	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> >>>+	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
> >>>  };
> >>These will be disabled since the msrs don't exist on the host.  See
> >>the comment above and KVM_SAVE_MSRS_BEGIN.
> >>
> >I see. Why not have two arrays?
> 
> Clearly better.
> 
> >>>+	case HV_X64_MSR_HYPERCALL: {
> >>>+		u64 gfn;
> >>>+		unsigned long addr;
> >>>+		/* if guest os id is not set hypercall should remain disabled */
> >>>+		if (!kvm->arch.hv_guest_os_id&&   data)
> >>>+			break;
> >>>+		kvm->arch.hv_hypercall = data;
> >>>+		if (!kvm_hv_hypercall_enabled(kvm))
> >>>+			break;
> >>>+		gfn = kvm->arch.hv_hypercall>>
> >>>+			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
> >>>+		addr = gfn_to_hva(kvm, gfn);
> >>>+		if (kvm_is_error_hva(addr))
> >>>+			return 1;
> >>Should di the error check before assigning, perhaps.
> >>
> >Spec doesn't tell. And guest will get #GP and BSOD anyway.
> 
> Well, all msrs I know of either #GP, or store the value and do what
> they're supposed to do, never both.
> 
> 
Make sense. Will fix.

--
			Gleb.

Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Avi Kivity]

On 01/17/2010 02:50 PM, Gleb Natapov wrote:
>>>> I prefer putting this in x86.c (maybe later split into hyperv.c).
>>>>
>>>>          
>>> This implements part of apic behaviour. It uses internal lapic functions
>>> like apic_reg_read()/apic_reg_write(). Why move it from lapic.c?
>>>        
>> The new functions implement hyper-v behaviour.  Why scatter them all around?
>>
>>      
> Each hyper-v extension is pretty much independent one from another, so
> why not group things by functionality instead. All apic related code in
> lapic.c.
>
>    
>> Maybe apic_reg_{read,write} need to be exported.
>>
>>      
> This is really internal API. It doesn't even check if apic is created.
>    

Okay.  We can rethink it later when the code grows some more.


-- 
error compiling committee.c: too many arguments to function