[PATCH 0/5] KVM: x86: 64/32 bit fixes and INIT/BSP fixes

[PATCH 0/5] KVM: x86: 64/32 bit fixes and INIT/BSP fixes

[Nadav Amit]

This patch-set handles 2 issues. Patches 1-3 deal with some more cases in which
bits [63:32] are not cleared when using dword opsize.  Patches 4-5 handle
anomalies with INIT/BSP (INIT does not behave exactly as reset).

Thanks for reviewing the patches.

Nadav Amit (5):
  KVM: x86: CMOV emulation on legacy mode is wrong
  KVM: x86: POPA emulation may not clear bits [63:32]
  KVM: x86: BSF and BSR emulation change register unnecassarily
  KVM: x86: INIT and reset sequences are different
  KVM: x86: BSP in MSR_IA32_APICBASE is writable

 arch/x86/include/asm/kvm_host.h |  6 ++--
 arch/x86/kvm/emulate.c          | 61 ++++++++++++++++++++++++++++-------------
 arch/x86/kvm/lapic.c            | 13 ++++-----
 arch/x86/kvm/lapic.h            |  2 +-
 arch/x86/kvm/svm.c              |  4 +--
 arch/x86/kvm/vmx.c              | 33 ++++++++++++----------
 arch/x86/kvm/x86.c              | 17 +++++++-----
 include/linux/kvm_host.h        |  7 ++++-
 8 files changed, 89 insertions(+), 54 deletions(-)

-- 
1.9.1

[PATCH 1/5] KVM: x86: CMOV emulation on legacy mode is wrong

[Nadav Amit]

On legacy mode CMOV emulation should still clear bits [63:32] even if the
assignment is not done. The previous fix 140bad89fd ("KVM: x86: emulation of
dword cmov on long-mode should clear [63:32]") was incomplete.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c941abe..62f7a39 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5126,8 +5126,7 @@ twobyte_insn:
 	case 0x40 ... 0x4f:	/* cmov */
 		if (test_cc(ctxt->b, ctxt->eflags))
 			ctxt->dst.val = ctxt->src.val;
-		else if (ctxt->mode != X86EMUL_MODE_PROT64 ||
-			 ctxt->op_bytes != 4)
+		else if (ctxt->op_bytes != 4)
 			ctxt->dst.type = OP_NONE; /* no writeback */
 		break;
 	case 0x80 ... 0x8f: /* jnz rel, etc*/
-- 
1.9.1

[PATCH 2/5] KVM: x86: POPA emulation may not clear bits [63:32]

[Nadav Amit]

POPA should assign the values to the registers as usual registers are assigned.
In other words, 32-bits register assignments should clear bits [63:32] of the
register.

Split the code of register assignments that will be used by future changes as
well.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 62f7a39..4961dc5 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -478,6 +478,25 @@ static void assign_masked(ulong *dest, ulong src, ulong mask)
 	*dest = (*dest & ~mask) | (src & mask);
 }
 
+static void assign_register(unsigned long *reg, u64 val, int bytes)
+{
+	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
+	switch (bytes) {
+	case 1:
+		*(u8 *)reg = (u8)val;
+		break;
+	case 2:
+		*(u16 *)reg = (u16)val;
+		break;
+	case 4:
+		*reg = (u32)val;
+		break;	/* 64b: zero-extend */
+	case 8:
+		*reg = val;
+		break;
+	}
+}
+
 static inline unsigned long ad_mask(struct x86_emulate_ctxt *ctxt)
 {
 	return (1UL << (ctxt->ad_bytes << 3)) - 1;
@@ -1691,21 +1710,7 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 
 static void write_register_operand(struct operand *op)
 {
-	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
-	switch (op->bytes) {
-	case 1:
-		*(u8 *)op->addr.reg = (u8)op->val;
-		break;
-	case 2:
-		*(u16 *)op->addr.reg = (u16)op->val;
-		break;
-	case 4:
-		*op->addr.reg = (u32)op->val;
-		break;	/* 64b: zero-extend */
-	case 8:
-		*op->addr.reg = op->val;
-		break;
-	}
+	return assign_register(op->addr.reg, op->val, op->bytes);
 }
 
 static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
@@ -1926,6 +1931,7 @@ static int em_popa(struct x86_emulate_ctxt *ctxt)
 {
 	int rc = X86EMUL_CONTINUE;
 	int reg = VCPU_REGS_RDI;
+	u32 val;
 
 	while (reg >= VCPU_REGS_RAX) {
 		if (reg == VCPU_REGS_RSP) {
@@ -1933,9 +1939,10 @@ static int em_popa(struct x86_emulate_ctxt *ctxt)
 			--reg;
 		}
 
-		rc = emulate_pop(ctxt, reg_rmw(ctxt, reg), ctxt->op_bytes);
+		rc = emulate_pop(ctxt, &val, ctxt->op_bytes);
 		if (rc != X86EMUL_CONTINUE)
 			break;
+		assign_register(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
 		--reg;
 	}
 	return rc;
-- 
1.9.1

[PATCH 3/5] KVM: x86: BSF and BSR emulation change register unnecassarily

[Nadav Amit]

If the source of BSF and BSR is zero, the destination register should not
change. That is how real hardware behaves.  If we set the destination even with
the same value that we had before, we may clear bits [63:32] unnecassarily.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4961dc5..7004577 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -962,6 +962,22 @@ FASTOP2(xadd);
 
 FASTOP2R(cmp, cmp_r);
 
+static int em_bsf_c(struct x86_emulate_ctxt *ctxt)
+{
+	/* If src is zero, do not writeback, but update flags */
+	if (ctxt->src.val == 0)
+		ctxt->dst.type = OP_NONE;
+	return fastop(ctxt, em_bsf);
+}
+
+static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
+{
+	/* If src is zero, do not writeback, but update flags */
+	if (ctxt->src.val == 0)
+		ctxt->dst.type = OP_NONE;
+	return fastop(ctxt, em_bsr);
+}
+
 static u8 test_cc(unsigned int condition, unsigned long flags)
 {
 	u8 rc;
@@ -4188,7 +4204,8 @@ static const struct opcode twobyte_table[256] = {
 	N, N,
 	G(BitOp, group8),
 	F(DstMem | SrcReg | ModRM | BitOp | Lock | PageTable, em_btc),
-	F(DstReg | SrcMem | ModRM, em_bsf), F(DstReg | SrcMem | ModRM, em_bsr),
+	I(DstReg | SrcMem | ModRM, em_bsf_c),
+	I(DstReg | SrcMem | ModRM, em_bsr_c),
 	D(DstReg | SrcMem8 | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xC0 - 0xC7 */
 	F2bv(DstMem | SrcReg | ModRM | SrcWrite | Lock, em_xadd),
-- 
1.9.1

[PATCH 4/5] KVM: x86: INIT and reset sequences are different

[Nadav Amit]

x86 architecture defines differences between the reset and INIT sequences.
INIT does not initialize the FPU (including MMX, XMM, YMM, etc.), TSC, PMU,
MSRs (in general), MTRRs machine-check, APIC ID, APIC arbitration ID and BSP.

References (from Intel SDM):

"If the MP protocol has completed and a BSP is chosen, subsequent INITs (either
to a specific processor or system wide) do not cause the MP protocol to be
repeated." [8.4.2: MP Initialization Protocol Requirements and Restrictions]

[Table 9-1. IA-32 Processor States Following Power-up, Reset, or INIT]

"If the processor is reset by asserting the INIT# pin, the x87 FPU state is not
changed." [9.2: X87 FPU INITIALIZATION]

"The state of the local APIC following an INIT reset is the same as it is after
a power-up or hardware reset, except that the APIC ID and arbitration ID
registers are not affected." [10.4.7.3: Local APIC State After an INIT Reset
(“Wait-for-SIPI” State)]

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/include/asm/kvm_host.h |  6 +++---
 arch/x86/kvm/lapic.c            | 11 ++++++-----
 arch/x86/kvm/lapic.h            |  2 +-
 arch/x86/kvm/svm.c              |  2 +-
 arch/x86/kvm/vmx.c              | 33 +++++++++++++++++++--------------
 arch/x86/kvm/x86.c              | 17 ++++++++++-------
 6 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bf5a160..59f4374 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -701,7 +701,7 @@ struct kvm_x86_ops {
 	/* Create, but do not attach this VCPU */
 	struct kvm_vcpu *(*vcpu_create)(struct kvm *kvm, unsigned id);
 	void (*vcpu_free)(struct kvm_vcpu *vcpu);
-	void (*vcpu_reset)(struct kvm_vcpu *vcpu);
+	void (*vcpu_reset)(struct kvm_vcpu *vcpu, bool init_event);
 
 	void (*prepare_guest_switch)(struct kvm_vcpu *vcpu);
 	void (*vcpu_load)(struct kvm_vcpu *vcpu, int cpu);
@@ -989,7 +989,7 @@ void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
 
 void kvm_inject_nmi(struct kvm_vcpu *vcpu);
 
-int fx_init(struct kvm_vcpu *vcpu);
+int fx_init(struct kvm_vcpu *vcpu, bool init_event);
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes);
@@ -1134,7 +1134,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
-void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event);
 void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu);
 void kvm_arch_mmu_notifier_invalidate_page(struct kvm *kvm,
 					   unsigned long address);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index bd4e34d..17da6fc 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1534,7 +1534,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 
 }
 
-void kvm_lapic_reset(struct kvm_vcpu *vcpu)
+void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct kvm_lapic *apic;
 	int i;
@@ -1548,7 +1548,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 	/* Stop the timer in case it's a reset to an active apic */
 	hrtimer_cancel(&apic->lapic_timer.timer);
 
-	kvm_apic_set_id(apic, vcpu->vcpu_id);
+	if (!init_event)
+		kvm_apic_set_id(apic, vcpu->vcpu_id);
 	kvm_apic_set_version(apic->vcpu);
 
 	for (i = 0; i < APIC_LVT_NUM; i++)
@@ -1689,7 +1690,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
 
 	static_key_slow_inc(&apic_sw_disabled.key); /* sw disabled at reset */
-	kvm_lapic_reset(vcpu);
+	kvm_lapic_reset(vcpu, false);
 	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
 
 	return 0;
@@ -2023,8 +2024,8 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	pe = xchg(&apic->pending_events, 0);
 
 	if (test_bit(KVM_APIC_INIT, &pe)) {
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
+		kvm_lapic_reset(vcpu, true);
+		kvm_vcpu_reset(vcpu, true);
 		if (kvm_vcpu_is_bsp(apic->vcpu))
 			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 		else
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 0bc6c65..e4c82dc 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -48,7 +48,7 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
-void kvm_lapic_reset(struct kvm_vcpu *vcpu);
+void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
 void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 155534c..1ef4c0d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1195,7 +1195,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	enable_gif(svm);
 }
 
-static void svm_vcpu_reset(struct kvm_vcpu *vcpu)
+static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u32 dummy;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index fdd9f8b..8aee6db 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4694,7 +4694,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	return 0;
 }
 
-static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
+static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct msr_data apic_base_msr;
@@ -4705,11 +4705,15 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 
 	vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
 	kvm_set_cr8(&vmx->vcpu, 0);
-	apic_base_msr.data = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
-	if (kvm_vcpu_is_bsp(&vmx->vcpu))
-		apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
-	apic_base_msr.host_initiated = true;
-	kvm_set_apic_base(&vmx->vcpu, &apic_base_msr);
+
+	if (!init_event) {
+		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
+				     MSR_IA32_APICBASE_ENABLE;
+		if (kvm_vcpu_is_bsp(&vmx->vcpu))
+			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
+		apic_base_msr.host_initiated = true;
+		kvm_set_apic_base(&vmx->vcpu, &apic_base_msr);
+	}
 
 	vmx_segment_cache_clear(vmx);
 
@@ -4733,9 +4737,12 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 	vmcs_write32(GUEST_LDTR_LIMIT, 0xffff);
 	vmcs_write32(GUEST_LDTR_AR_BYTES, 0x00082);
 
-	vmcs_write32(GUEST_SYSENTER_CS, 0);
-	vmcs_writel(GUEST_SYSENTER_ESP, 0);
-	vmcs_writel(GUEST_SYSENTER_EIP, 0);
+	if (!init_event) {
+		vmcs_write32(GUEST_SYSENTER_CS, 0);
+		vmcs_writel(GUEST_SYSENTER_ESP, 0);
+		vmcs_writel(GUEST_SYSENTER_EIP, 0);
+		vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
+	}
 
 	vmcs_writel(GUEST_RFLAGS, 0x02);
 	kvm_rip_write(vcpu, 0xfff0);
@@ -4750,14 +4757,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, 0);
 	vmcs_write32(GUEST_PENDING_DBG_EXCEPTIONS, 0);
 
-	/* Special registers */
-	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
-
 	setup_msrs(vmx);
 
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
 
-	if (cpu_has_vmx_tpr_shadow()) {
+	if (cpu_has_vmx_tpr_shadow() && !init_event) {
 		vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, 0);
 		if (vm_need_tpr_shadow(vmx->vcpu.kvm))
 			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR,
@@ -4776,7 +4780,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
 	vmx_set_cr4(&vmx->vcpu, 0);
-	vmx_set_efer(&vmx->vcpu, 0);
+	if (!init_event)
+		vmx_set_efer(&vmx->vcpu, 0);
 	vmx_fpu_activate(&vmx->vcpu);
 	update_exception_bitmap(&vmx->vcpu);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cc2c759..324e639 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6961,7 +6961,7 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 	return 0;
 }
 
-int fx_init(struct kvm_vcpu *vcpu)
+int fx_init(struct kvm_vcpu *vcpu, bool init_event)
 {
 	int err;
 
@@ -6969,7 +6969,9 @@ int fx_init(struct kvm_vcpu *vcpu)
 	if (err)
 		return err;
 
-	fpu_finit(&vcpu->arch.guest_fpu);
+	if (!init_event)
+		fpu_finit(&vcpu->arch.guest_fpu);
+
 	if (cpu_has_xsaves)
 		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
 			host_xcr0 | XSTATE_COMPACTION_ENABLED;
@@ -7049,7 +7051,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 	r = vcpu_load(vcpu);
 	if (r)
 		return r;
-	kvm_vcpu_reset(vcpu);
+	kvm_vcpu_reset(vcpu, false);
 	kvm_mmu_setup(vcpu);
 	vcpu_put(vcpu);
 
@@ -7087,7 +7089,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->vcpu_free(vcpu);
 }
 
-void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	atomic_set(&vcpu->arch.nmi_queued, 0);
 	vcpu->arch.nmi_pending = 0;
@@ -7111,13 +7113,14 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
 
-	kvm_pmu_reset(vcpu);
+	if (!init_event)
+		kvm_pmu_reset(vcpu);
 
 	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
 	vcpu->arch.regs_avail = ~0;
 	vcpu->arch.regs_dirty = ~0;
 
-	kvm_x86_ops->vcpu_reset(vcpu);
+	kvm_x86_ops->vcpu_reset(vcpu, init_event);
 }
 
 void kvm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
@@ -7299,7 +7302,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 		goto fail_free_mce_banks;
 	}
 
-	r = fx_init(vcpu);
+	r = fx_init(vcpu, false);
 	if (r)
 		goto fail_free_wbinvd_dirty_mask;
 
-- 
1.9.1

[PATCH 5/5] KVM: x86: BSP in MSR_IA32_APICBASE is writable

[Nadav Amit]

After reset, the CPU can change the BSP, which will be used upon INIT.  Reset
should return the BSP which QEMU asked for, and therefore handled accordingly.

To quote: "If the MP protocol has completed and a BSP is chosen, subsequent
INITs (either to a specific processor or system wide) do not cause the MP
protocol to be repeated."
[Intel SDM 8.4.2: MP Initialization Protocol Requirements and Restrictions]

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/lapic.c     | 2 --
 arch/x86/kvm/svm.c       | 2 +-
 arch/x86/kvm/vmx.c       | 2 +-
 include/linux/kvm_host.h | 7 ++++++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 17da6fc..b0dbf68 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1498,8 +1498,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 		return;
 	}
 
-	if (!kvm_vcpu_is_bsp(apic->vcpu))
-		value &= ~MSR_IA32_APICBASE_BSP;
 	vcpu->arch.apic_base = value;
 
 	/* update jump label if enable bit changes */
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1ef4c0d..ef5bf21 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1261,7 +1261,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 
 	svm->vcpu.arch.apic_base = APIC_DEFAULT_PHYS_BASE |
 				   MSR_IA32_APICBASE_ENABLE;
-	if (kvm_vcpu_is_bsp(&svm->vcpu))
+	if (kvm_vcpu_is_reset_bsp(&svm->vcpu))
 		svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP;
 
 	svm_init_osvw(&svm->vcpu);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 8aee6db..7e370b2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4709,7 +4709,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (!init_event) {
 		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
 				     MSR_IA32_APICBASE_ENABLE;
-		if (kvm_vcpu_is_bsp(&vmx->vcpu))
+		if (kvm_vcpu_is_reset_bsp(&vmx->vcpu))
 			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
 		apic_base_msr.host_initiated = true;
 		kvm_set_apic_base(&vmx->vcpu, &apic_base_msr);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 0f574eb..8365cae 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -968,11 +968,16 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 #endif /* CONFIG_HAVE_KVM_EVENTFD */
 
 #ifdef CONFIG_KVM_APIC_ARCHITECTURE
-static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
+static inline bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
 {
 	return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id;
 }
 
+static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
+{
+	return (vcpu->arch.apic_base & MSR_IA32_APICBASE_BSP) != 0;
+}
+
 bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu);
 
 #else
-- 
1.9.1

Re: [PATCH 5/5] KVM: x86: BSP in MSR_IA32_APICBASE is writable

[Paolo Bonzini]

On 30/03/2015 14:39, Nadav Amit wrote:
> After reset, the CPU can change the BSP, which will be used upon INIT.  Reset
> should return the BSP which QEMU asked for, and therefore handled accordingly.
> 
> To quote: "If the MP protocol has completed and a BSP is chosen, subsequent
> INITs (either to a specific processor or system wide) do not cause the MP
> protocol to be repeated."
> [Intel SDM 8.4.2: MP Initialization Protocol Requirements and Restrictions]
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>

Please provide a kvm-unit-tests testcase for this.

Paolo

> ---
>  arch/x86/kvm/lapic.c     | 2 --
>  arch/x86/kvm/svm.c       | 2 +-
>  arch/x86/kvm/vmx.c       | 2 +-
>  include/linux/kvm_host.h | 7 ++++++-
>  4 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 17da6fc..b0dbf68 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1498,8 +1498,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>  		return;
>  	}
>  
> -	if (!kvm_vcpu_is_bsp(apic->vcpu))
> -		value &= ~MSR_IA32_APICBASE_BSP;
>  	vcpu->arch.apic_base = value;
>  
>  	/* update jump label if enable bit changes */
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1ef4c0d..ef5bf21 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1261,7 +1261,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
>  
>  	svm->vcpu.arch.apic_base = APIC_DEFAULT_PHYS_BASE |
>  				   MSR_IA32_APICBASE_ENABLE;
> -	if (kvm_vcpu_is_bsp(&svm->vcpu))
> +	if (kvm_vcpu_is_reset_bsp(&svm->vcpu))
>  		svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP;
>  
>  	svm_init_osvw(&svm->vcpu);
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 8aee6db..7e370b2 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4709,7 +4709,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	if (!init_event) {
>  		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
>  				     MSR_IA32_APICBASE_ENABLE;
> -		if (kvm_vcpu_is_bsp(&vmx->vcpu))
> +		if (kvm_vcpu_is_reset_bsp(&vmx->vcpu))
>  			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
>  		apic_base_msr.host_initiated = true;
>  		kvm_set_apic_base(&vmx->vcpu, &apic_base_msr);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0f574eb..8365cae 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -968,11 +968,16 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  #endif /* CONFIG_HAVE_KVM_EVENTFD */
>  
>  #ifdef CONFIG_KVM_APIC_ARCHITECTURE
> -static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
> +static inline bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id;
>  }
>  
> +static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
> +{
> +	return (vcpu->arch.apic_base & MSR_IA32_APICBASE_BSP) != 0;
> +}
> +
>  bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu);
>  
>  #else
> 

Re: [PATCH 5/5] KVM: x86: BSP in MSR_IA32_APICBASE is writable

[Nadav Amit]

Paolo,

It appears you are right and I have not tested 4 and 5 well enough. I’ll
repost them (the others were tested presumably well enough).

Two short questions:
Can I use init.c in the kvm-unit-tests ? 
Why is it disabled?

Nadav


Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 30/03/2015 14:39, Nadav Amit wrote:
>> After reset, the CPU can change the BSP, which will be used upon INIT.  Reset
>> should return the BSP which QEMU asked for, and therefore handled accordingly.
>> 
>> To quote: "If the MP protocol has completed and a BSP is chosen, subsequent
>> INITs (either to a specific processor or system wide) do not cause the MP
>> protocol to be repeated."
>> [Intel SDM 8.4.2: MP Initialization Protocol Requirements and Restrictions]
>> 
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> 
> Please provide a kvm-unit-tests testcase for this.
> 
> Paolo
> 
>> ---
>> arch/x86/kvm/lapic.c     | 2 --
>> arch/x86/kvm/svm.c       | 2 +-
>> arch/x86/kvm/vmx.c       | 2 +-
>> include/linux/kvm_host.h | 7 ++++++-
>> 4 files changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 17da6fc..b0dbf68 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1498,8 +1498,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>> 		return;
>> 	}
>> 
>> -	if (!kvm_vcpu_is_bsp(apic->vcpu))
>> -		value &= ~MSR_IA32_APICBASE_BSP;
>> 	vcpu->arch.apic_base = value;
>> 
>> 	/* update jump label if enable bit changes */
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 1ef4c0d..ef5bf21 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1261,7 +1261,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
>> 
>> 	svm->vcpu.arch.apic_base = APIC_DEFAULT_PHYS_BASE |
>> 				   MSR_IA32_APICBASE_ENABLE;
>> -	if (kvm_vcpu_is_bsp(&svm->vcpu))
>> +	if (kvm_vcpu_is_reset_bsp(&svm->vcpu))
>> 		svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP;
>> 
>> 	svm_init_osvw(&svm->vcpu);
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 8aee6db..7e370b2 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4709,7 +4709,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>> 	if (!init_event) {
>> 		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
>> 				     MSR_IA32_APICBASE_ENABLE;
>> -		if (kvm_vcpu_is_bsp(&vmx->vcpu))
>> +		if (kvm_vcpu_is_reset_bsp(&vmx->vcpu))
>> 			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
>> 		apic_base_msr.host_initiated = true;
>> 		kvm_set_apic_base(&vmx->vcpu, &apic_base_msr);
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 0f574eb..8365cae 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -968,11 +968,16 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>> #endif /* CONFIG_HAVE_KVM_EVENTFD */
>> 
>> #ifdef CONFIG_KVM_APIC_ARCHITECTURE
>> -static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
>> +static inline bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
>> {
>> 	return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id;
>> }
>> 
>> +static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
>> +{
>> +	return (vcpu->arch.apic_base & MSR_IA32_APICBASE_BSP) != 0;
>> +}
>> +
>> bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu);
>> 
>> #else
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] KVM: x86: BSP in MSR_IA32_APICBASE is writable

[Paolo Bonzini]

On 30/03/2015 16:40, Nadav Amit wrote:
> Paolo,
> 
> It appears you are right and I have not tested 4 and 5 well enough. I’ll
> repost them (the others were tested presumably well enough).
> 
> Two short questions:
> Can I use init.c in the kvm-unit-tests ? 
> Why is it disabled?

Because QEMU support for INIT is incomplete, so the tests would fail
("Uh, hard reset!").  IIRC sending init to BSP with APIC_DEST_SELF is
also not supported by actual hardware (or at least not supported
"officially") so that test would also have to be changed to not use a
shortcut.

Paolo

Re: [PATCH 0/5] KVM: x86: 64/32 bit fixes and INIT/BSP fixes

[Paolo Bonzini]

On 30/03/2015 14:39, Nadav Amit wrote:
> This patch-set handles 2 issues. Patches 1-3 deal with some more cases in which
> bits [63:32] are not cleared when using dword opsize.  Patches 4-5 handle
> anomalies with INIT/BSP (INIT does not behave exactly as reset).
> 
> Thanks for reviewing the patches.
> 
> Nadav Amit (5):
>   KVM: x86: CMOV emulation on legacy mode is wrong
>   KVM: x86: POPA emulation may not clear bits [63:32]
>   KVM: x86: BSF and BSR emulation change register unnecassarily
>   KVM: x86: INIT and reset sequences are different
>   KVM: x86: BSP in MSR_IA32_APICBASE is writable
> 
>  arch/x86/include/asm/kvm_host.h |  6 ++--
>  arch/x86/kvm/emulate.c          | 61 ++++++++++++++++++++++++++++-------------
>  arch/x86/kvm/lapic.c            | 13 ++++-----
>  arch/x86/kvm/lapic.h            |  2 +-
>  arch/x86/kvm/svm.c              |  4 +--
>  arch/x86/kvm/vmx.c              | 33 ++++++++++++----------
>  arch/x86/kvm/x86.c              | 17 +++++++-----
>  include/linux/kvm_host.h        |  7 ++++-
>  8 files changed, 89 insertions(+), 54 deletions(-)
> 

Applying patches 1-3.

Paolo

Re: [PATCH 5/5] KVM: x86: BSP in MSR_IA32_APICBASE is writable

[Nadav Amit]

Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 30/03/2015 16:40, Nadav Amit wrote:
>> Paolo,
>> 
>> It appears you are right and I have not tested 4 and 5 well enough. I’ll
>> repost them (the others were tested presumably well enough).
>> 
>> Two short questions:
>> Can I use init.c in the kvm-unit-tests ? 
>> Why is it disabled?
> 
> Because QEMU support for INIT is incomplete, so the tests would fail
> ("Uh, hard reset!").  IIRC sending init to BSP with APIC_DEST_SELF is
> also not supported by actual hardware (or at least not supported
> "officially") so that test would also have to be changed to not use a
> shortcut.

So, I would revive the init unit-test and disable the failing assertions,
right?

BTW: It appears that there is another bug - DR[0..3] are not reloaded after
reset.

Regards,
Nadav

-- >8 --

From: Nadav Amit <namit@cs.technion.ac.il>
Subject: [PATCH] KVM: x86: DR0-DR3 are not clear on reset

DR0-DR3 are not cleared as they should during reset and when they are set from
userspace.  It appears to be caused by c77fb5fe6f03 ("KVM: x86: Allow the guest
to run with dirty debug registers").

Force their reload on these situations.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/x86.c              | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bf5a160..913ae41 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -345,6 +345,7 @@ struct kvm_pmu {
 enum {
 	KVM_DEBUGREG_BP_ENABLED = 1,
 	KVM_DEBUGREG_WONT_EXIT = 2,
+	KVM_DEBUGREG_RELOAD = 4,
 };
 
 struct kvm_vcpu_arch {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cc2c759..1f65c3a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -801,6 +801,17 @@ unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_get_cr8);
 
+static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
+{
+	int i;
+
+	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
+		for (i = 0; i < KVM_NR_DB_REGS; i++)
+			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
+		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
+	}
+}
+
 static void kvm_update_dr6(struct kvm_vcpu *vcpu)
 {
 	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
@@ -3150,6 +3161,7 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	memcpy(vcpu->arch.db, dbgregs->db, sizeof(vcpu->arch.db));
+	kvm_update_dr0123(vcpu);
 	vcpu->arch.dr6 = dbgregs->dr6;
 	kvm_update_dr6(vcpu);
 	vcpu->arch.dr7 = dbgregs->dr7;
@@ -6322,6 +6334,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		set_debugreg(vcpu->arch.eff_db[2], 2);
 		set_debugreg(vcpu->arch.eff_db[3], 3);
 		set_debugreg(vcpu->arch.dr6, 6);
+		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
 	}
 
 	trace_kvm_entry(vcpu->vcpu_id);
@@ -7096,6 +7109,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
 	kvm_clear_exception_queue(vcpu);
 
 	memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
+	kvm_update_dr0123(vcpu);
 	vcpu->arch.dr6 = DR6_INIT;
 	kvm_update_dr6(vcpu);
 	vcpu->arch.dr7 = DR7_FIXED_1;
-- 
1.9.1

Re: [PATCH 5/5] KVM: x86: BSP in MSR_IA32_APICBASE is writable

[Paolo Bonzini]

On 30/03/2015 21:31, Nadav Amit wrote:
> > Because QEMU support for INIT is incomplete, so the tests would fail
> > ("Uh, hard reset!").  IIRC sending init to BSP with APIC_DEST_SELF is
> > also not supported by actual hardware (or at least not supported
> > "officially") so that test would also have to be changed to not use a
> > shortcut.
> 
> So, I would revive the init unit-test and disable the failing assertions,
> right?

Yes, or otherwise mark them as expected failures so that they do not
affect the exit code.

Paolo

> BTW: It appears that there is another bug - DR[0..3] are not reloaded after
> reset.