All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] KVM: x86: Additional rflags.rf fixes
@ 2014-07-21 11:37 Nadav Amit
  2014-07-21 11:37 ` [PATCH 1/7] KVM: x86: Defining missing x86 vectors Nadav Amit
                   ` (8 more replies)
  0 siblings, 9 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

RFLAGS.RF is not handled well by kvm, in both the x86 emulator and vmx code.
This flag should be cleared after every instruction emulation (other than
IRETD/IRETQ).  It should be set in various conditions as described in Intel SDM
17.3.1.1.  This series of patches addresses the clearing of RF on emulated
instructions, the setting the RF upon fault injection.  It does not handle the
case of traps and interrupts injection during REP-string, since there is
no easy indication whether the first iteration of a rep-string occurred.
The value of RF depends on whether the first iteration took place.

Thanks for reviewing the patches.

Nadav Amit (7):
  KVM: x86: Defining missing x86 vectors
  KVM: x86: Function for determining exception type
  KVM: x86: Clearing rflags.rf upon skipped emulated instruction
  KVM: vmx: set rflags.rf during fault injection
  KVM: x86: popf emulation should not change RF
  KVM: x86: Clear rflags.rf on emulated instructions
  KVM: x86: Cleanup of rflags.rf cleaning

 arch/x86/include/uapi/asm/kvm.h |  3 +++
 arch/x86/kvm/emulate.c          | 13 ++++++++-----
 arch/x86/kvm/vmx.c              | 11 ++++++++++-
 arch/x86/kvm/x86.c              | 37 +++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.h              |  9 +++++++++
 5 files changed, 67 insertions(+), 6 deletions(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/7] KVM: x86: Defining missing x86 vectors
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 11:37 ` [PATCH 2/7] KVM: x86: Function for determining exception type Nadav Amit
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

Defining XE, XM and VE vector numbers.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/include/uapi/asm/kvm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index d3a8778..d7dcef5 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -23,7 +23,10 @@
 #define GP_VECTOR 13
 #define PF_VECTOR 14
 #define MF_VECTOR 16
+#define AC_VECTOR 17
 #define MC_VECTOR 18
+#define XM_VECTOR 19
+#define VE_VECTOR 20
 
 /* Select x86 specific features in <linux/kvm.h> */
 #define __KVM_HAVE_PIT
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/7] KVM: x86: Function for determining exception type
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
  2014-07-21 11:37 ` [PATCH 1/7] KVM: x86: Defining missing x86 vectors Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 12:18   ` Paolo Bonzini
  2014-07-21 11:37 ` [PATCH 3/7] KVM: x86: Clearing rflags.rf upon skipped emulated instruction Nadav Amit
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

New function for determining the x86 exception type: fault, abort, trap, etc.
This function is used by the next patch for setting rflags.rf upon faults.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/x86.c | 35 +++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.h |  9 +++++++++
 2 files changed, 44 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f750b69..c2aa58e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -311,6 +311,41 @@ static int exception_class(int vector)
 	return EXCPT_BENIGN;
 }
 
+int kvm_exception_type(unsigned int nr)
+{
+	switch (nr) {
+	case DE_VECTOR:
+	case BR_VECTOR:
+	case UD_VECTOR:
+	case NM_VECTOR:
+	case TS_VECTOR:
+	case NP_VECTOR:
+	case SS_VECTOR:
+	case GP_VECTOR:
+	case PF_VECTOR:
+	case MF_VECTOR:
+	case AC_VECTOR:
+	case XM_VECTOR:
+	case VE_VECTOR:
+		return EXCPT_FAULT;
+	case DB_VECTOR:
+		return EXCPT_FAULT_OR_TRAP;
+	case BP_VECTOR:
+	case OF_VECTOR:
+		return EXCPT_TRAP;
+	case DF_VECTOR:
+	case MC_VECTOR:
+		return EXCPT_ABORT;
+	case 15:
+	case 21 ... 31:
+		return EXCPT_RESERVED;
+	default:
+		break;
+	}
+	return EXCPT_INTERRUPT;
+}
+EXPORT_SYMBOL_GPL(kvm_exception_type);
+
 static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
 		unsigned nr, bool has_error, u32 error_code,
 		bool reinject)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 306a1b7..b413181 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -151,6 +151,15 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
 
 #define KVM_SUPPORTED_XCR0     (XSTATE_FP | XSTATE_SSE | XSTATE_YMM \
 				| XSTATE_BNDREGS | XSTATE_BNDCSR)
+
+#define EXCPT_FAULT		0
+#define EXCPT_TRAP		1
+#define EXCPT_FAULT_OR_TRAP	2
+#define EXCPT_ABORT		3
+#define EXCPT_RESERVED		4
+#define EXCPT_INTERRUPT		5
+int kvm_exception_type(unsigned int nr);
+
 extern u64 host_xcr0;
 
 extern u64 kvm_supported_xcr0(void);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/7] KVM: x86: Clearing rflags.rf upon skipped emulated instruction
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
  2014-07-21 11:37 ` [PATCH 1/7] KVM: x86: Defining missing x86 vectors Nadav Amit
  2014-07-21 11:37 ` [PATCH 2/7] KVM: x86: Function for determining exception type Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 11:37 ` [PATCH 4/7] KVM: vmx: set rflags.rf during fault injection Nadav Amit
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

When skipping an emulated instruction, rflags.rf should be cleared as it would
be on real x86 CPU.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c2aa58e..120ee83 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5264,6 +5264,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 
 	if (emulation_type & EMULTYPE_SKIP) {
 		kvm_rip_write(vcpu, ctxt->_eip);
+		if (ctxt->eflags & X86_EFLAGS_RF)
+			kvm_set_rflags(vcpu, ctxt->eflags & ~X86_EFLAGS_RF);
 		return EMULATE_DONE;
 	}
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/7] KVM: vmx: set rflags.rf during fault injection
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
                   ` (2 preceding siblings ...)
  2014-07-21 11:37 ` [PATCH 3/7] KVM: x86: Clearing rflags.rf upon skipped emulated instruction Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 12:05   ` Paolo Bonzini
  2014-07-21 11:37 ` [PATCH 5/7] KVM: x86: popf emulation should not change RF Nadav Amit
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

VMX does not automatically set rflags.rf during event injection. This patch
does partial job, setting rflags.rf upon fault injection.  It also marks that
injection of trap/interrupt during rep-string instruction is not properly
emulated. It is unclear how to do it efficiently without decoding the guest
instruction before interrupt injection.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 0c9569b..8edb785 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2006,6 +2006,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 				bool reinject)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	unsigned long rflags;
 	u32 intr_info = nr | INTR_INFO_VALID_MASK;
 
 	if (!reinject && is_guest_mode(vcpu) &&
@@ -2017,6 +2018,12 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 		intr_info |= INTR_INFO_DELIVER_CODE_MASK;
 	}
 
+	rflags = vmx_get_rflags(vcpu);
+	if (kvm_exception_type(nr) == EXCPT_FAULT)
+		vmx_set_rflags(vcpu, rflags | X86_EFLAGS_RF);
+
+	/* TODO: Set rflags.rf on trap during rep-string */
+
 	if (vmx->rmode.vm86_active) {
 		int inc_eip = 0;
 		if (kvm_exception_is_soft(nr))
@@ -4631,8 +4638,10 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
 		intr |= INTR_TYPE_SOFT_INTR;
 		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN,
 			     vmx->vcpu.arch.event_exit_inst_len);
-	} else
+	} else {
+		/* TODO: Set rflags.rf during rep-string */
 		intr |= INTR_TYPE_EXT_INTR;
+	}
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr);
 }
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 5/7] KVM: x86: popf emulation should not change RF
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
                   ` (3 preceding siblings ...)
  2014-07-21 11:37 ` [PATCH 4/7] KVM: vmx: set rflags.rf during fault injection Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 11:37 ` [PATCH 6/7] KVM: x86: Clear rflags.rf on emulated instructions Nadav Amit
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

RFLAGS.RF is always zero after popf. Therefore, popf should not updated RF, as
anyhow emulating popf, just as any other instruction should clear RFLAGS.RF.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index dd07410..cf117bf 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1675,7 +1675,7 @@ static int emulate_popf(struct x86_emulate_ctxt *ctxt,
 		return rc;
 
 	change_mask = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF | EFLG_OF
-		| EFLG_TF | EFLG_DF | EFLG_NT | EFLG_RF | EFLG_AC | EFLG_ID;
+		| EFLG_TF | EFLG_DF | EFLG_NT | EFLG_AC | EFLG_ID;
 
 	switch(ctxt->mode) {
 	case X86EMUL_MODE_PROT64:
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 6/7] KVM: x86: Clear rflags.rf on emulated instructions
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
                   ` (4 preceding siblings ...)
  2014-07-21 11:37 ` [PATCH 5/7] KVM: x86: popf emulation should not change RF Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 11:37 ` [PATCH 7/7] KVM: x86: Cleanup of rflags.rf cleaning Nadav Amit
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

When an instruction is emulated RFLAGS.RF should be cleared. KVM previously did
not do so. This patch clears RFLAGS.RF after interception is done.  If a fault
occurs during the instruction, RFLAGS.RF will be set by a previous patch.  This
patch does not handle the case of traps/interrupts during rep-strings. Traps
are only expected to occur on debug watchpoints, and those are anyhow not
handled by the emulator.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index cf117bf..189b8bd8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4640,6 +4640,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 			/* All REP prefixes have the same first termination condition */
 			if (address_mask(ctxt, reg_read(ctxt, VCPU_REGS_RCX)) == 0) {
 				ctxt->eip = ctxt->_eip;
+				ctxt->eflags &= ~EFLG_RF;
 				goto done;
 			}
 		}
@@ -4682,6 +4683,8 @@ special_insn:
 			goto done;
 	}
 
+	ctxt->eflags &= ~EFLG_RF;
+
 	if (ctxt->execute) {
 		if (ctxt->d & Fastop) {
 			void (*fop)(struct fastop *) = (void *)ctxt->execute;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 7/7] KVM: x86: Cleanup of rflags.rf cleaning
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
                   ` (5 preceding siblings ...)
  2014-07-21 11:37 ` [PATCH 6/7] KVM: x86: Clear rflags.rf on emulated instructions Nadav Amit
@ 2014-07-21 11:37 ` Nadav Amit
  2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
  2014-07-21 12:19 ` [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Paolo Bonzini
  8 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:37 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

RFLAGS.RF was cleaned in several functions (e.g., syscall) in the x86 emulator.
Now that we clear it before the execution of an instruction in the emulator, we
can remove the specific cleanup of RFLAGS.RF.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 189b8bd8..8d41556 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2211,7 +2211,7 @@ static int em_syscall(struct x86_emulate_ctxt *ctxt)
 	*reg_write(ctxt, VCPU_REGS_RCX) = ctxt->_eip;
 	if (efer & EFER_LMA) {
 #ifdef CONFIG_X86_64
-		*reg_write(ctxt, VCPU_REGS_R11) = ctxt->eflags & ~EFLG_RF;
+		*reg_write(ctxt, VCPU_REGS_R11) = ctxt->eflags;
 
 		ops->get_msr(ctxt,
 			     ctxt->mode == X86EMUL_MODE_PROT64 ?
@@ -2219,14 +2219,14 @@ static int em_syscall(struct x86_emulate_ctxt *ctxt)
 		ctxt->_eip = msr_data;
 
 		ops->get_msr(ctxt, MSR_SYSCALL_MASK, &msr_data);
-		ctxt->eflags &= ~(msr_data | EFLG_RF);
+		ctxt->eflags &= ~msr_data;
 #endif
 	} else {
 		/* legacy mode */
 		ops->get_msr(ctxt, MSR_STAR, &msr_data);
 		ctxt->_eip = (u32)msr_data;
 
-		ctxt->eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
+		ctxt->eflags &= ~(EFLG_VM | EFLG_IF);
 	}
 
 	return X86EMUL_CONTINUE;
@@ -2275,7 +2275,7 @@ static int em_sysenter(struct x86_emulate_ctxt *ctxt)
 		break;
 	}
 
-	ctxt->eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
+	ctxt->eflags &= ~(EFLG_VM | EFLG_IF);
 	cs_sel = (u16)msr_data;
 	cs_sel &= ~SELECTOR_RPL_MASK;
 	ss_sel = cs_sel + 8;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
                   ` (6 preceding siblings ...)
  2014-07-21 11:37 ` [PATCH 7/7] KVM: x86: Cleanup of rflags.rf cleaning Nadav Amit
@ 2014-07-21 11:39 ` Nadav Amit
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 1/3] x86: Check rflags.rf is cleared after emulation Nadav Amit
                     ` (3 more replies)
  2014-07-21 12:19 ` [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Paolo Bonzini
  8 siblings, 4 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:39 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

This series of patches introduces checks for rflags.rf and whether it is
cleared after emulation, and set correctly.  The last (third) patch fails even
with recent fixes, since there is no easy way for the hypervisor to determine
whether any iteration of rep-string was executed before. RFLAGS.RF should be
cleared before the first iteration, and set otherwise.

Nadav Amit (3):
  x86: Check rflags.rf is cleared after emulation
  x86: Test rflags.rf is set upon faults
  x86: Check RFLAGS.RF on interrupt during REP-str

 lib/x86/desc.c | 14 +++++++++++---
 lib/x86/desc.h |  1 +
 x86/eventinj.c | 16 ++++++++++++++++
 x86/idt_test.c | 13 +++++++++----
 x86/realmode.c |  3 ++-
 5 files changed, 39 insertions(+), 8 deletions(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH kvm-unit-tests 1/3] x86: Check rflags.rf is cleared after emulation
  2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
@ 2014-07-21 11:39   ` Nadav Amit
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 2/3] x86: Test rflags.rf is set upon faults Nadav Amit
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:39 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

RFLAGS.RF should be cleared after every instruction emulation. Recently
discovered bug indicated this is not the case. This patch adds a test to check
this behavior. It is done by setting RF, executing IRET and checking whether
the saved RF is cleared. Since the flags are saved several instructions after
IRET is executed, RF should be cleared.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/realmode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/x86/realmode.c b/x86/realmode.c
index 10c3e03..09e6aa7 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -744,7 +744,7 @@ void test_iret()
 	MK_INSN(iret_flags32, "pushfl\n\t"
 			      "popl %eax\n\t"
 			      "andl $~0x2, %eax\n\t"
-			      "orl $0xffc08028, %eax\n\t"
+			      "orl $0xffc18028, %eax\n\t"
 			      "pushl %eax\n\t"
 			      "pushl %cs\n\t"
 			      "call 1f\n\t"
@@ -773,6 +773,7 @@ void test_iret()
 
 	exec_in_big_real_mode(&insn_iret_flags32);
 	report("iret 3", R_AX, 1);
+	report("rflags.rf", ~0, !(outregs.eflags & (1 << 16)));
 
 	exec_in_big_real_mode(&insn_iret_flags16);
 	report("iret 4", R_AX, 1);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH kvm-unit-tests 2/3] x86: Test rflags.rf is set upon faults
  2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 1/3] x86: Check rflags.rf is cleared after emulation Nadav Amit
@ 2014-07-21 11:39   ` Nadav Amit
  2014-07-21 12:24     ` Paolo Bonzini
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 3/3] x86: Check RFLAGS.RF on interrupt during REP-str Nadav Amit
  2014-07-21 12:25   ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Paolo Bonzini
  3 siblings, 1 reply; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:39 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

This patch tests whether rflags.rf is set upon #UD and #GP faults as it should,
according to Intel SDM 17.3.1.1.  The patch saves rflags.rf in an unused bit of
the value which is saved during exception handling to save rflags.rf.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 lib/x86/desc.c | 14 +++++++++++---
 lib/x86/desc.h |  1 +
 x86/idt_test.c | 13 +++++++++----
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 9a80f48..9e5d66a 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -36,8 +36,8 @@ static void check_exception_table(struct ex_regs *regs)
     struct ex_record *ex;
     unsigned ex_val;
 
-    ex_val = regs->vector | (regs->error_code << 16);
-
+    ex_val = regs->vector | (regs->error_code << 16) |
+		(((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));
 
     for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
@@ -176,7 +176,7 @@ unsigned exception_vector(void)
     unsigned short vector;
 
     asm("mov %%gs:4, %0" : "=rm"(vector));
-    return vector;
+    return (u8)vector;
 }
 
 unsigned exception_error_code(void)
@@ -187,6 +187,14 @@ unsigned exception_error_code(void)
     return error_code;
 }
 
+bool exception_rflags_rf(void)
+{
+    unsigned short rf_flag;
+
+    asm("mov %%gs:4, %0" : "=rm"(rf_flag));
+    return (rf_flag >> 8) & 1;
+}
+
 static char intr_alt_stack[4096];
 
 #ifndef __x86_64__
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 553bce9..bd4293e 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -144,6 +144,7 @@ extern tss64_t tss;
 
 unsigned exception_vector(void);
 unsigned exception_error_code(void);
+bool exception_rflags_rf(void);
 void set_idt_entry(int vec, void *addr, int dpl);
 void set_idt_sel(int vec, u16 sel);
 void set_gdt_entry(int sel, u32 base,  u32 limit, u8 access, u8 gran);
diff --git a/x86/idt_test.c b/x86/idt_test.c
index ecb76bb..349aade 100644
--- a/x86/idt_test.c
+++ b/x86/idt_test.c
@@ -1,15 +1,16 @@
 #include "libcflat.h"
 #include "desc.h"
 
-int test_ud2(void)
+int test_ud2(bool *rflags_rf)
 {
     asm volatile(ASM_TRY("1f")
                  "ud2 \n\t"
                  "1:" :);
+    *rflags_rf = exception_rflags_rf();
     return exception_vector();
 }
 
-int test_gp(void)
+int test_gp(bool *rflags_rf)
 {
     unsigned long tmp;
 
@@ -18,19 +19,23 @@ int test_gp(void)
 		 "mov %0, %%cr4\n\t"
                  "1:"
                  : "=a"(tmp));
+    *rflags_rf = exception_rflags_rf();
     return exception_vector();
 }
 
 int main(void)
 {
     int r;
+    bool rflags_rf;
 
     printf("Starting IDT test\n");
     setup_idt();
-    r = test_gp();
+    r = test_gp(&rflags_rf);
     report("Testing #GP", r == GP_VECTOR);
-    r = test_ud2();
+    report("Testing #GP rflags.rf", rflags_rf);
+    r = test_ud2(&rflags_rf);
     report("Testing #UD", r == UD_VECTOR);
+    report("Testing #UD rflags.rf", rflags_rf);
 
     return report_summary();
 }
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH kvm-unit-tests 3/3] x86: Check RFLAGS.RF on interrupt during REP-str
  2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 1/3] x86: Check rflags.rf is cleared after emulation Nadav Amit
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 2/3] x86: Test rflags.rf is set upon faults Nadav Amit
@ 2014-07-21 11:39   ` Nadav Amit
  2014-07-21 12:25   ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Paolo Bonzini
  3 siblings, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 11:39 UTC (permalink / raw)
  To: pbonzini; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel, Nadav Amit

Intel SDM states (17.3.1.1): "For any interrupt arriving after any iteration of
a repeated string instruction but the last iteration, the value pushed for RF
is 1." This test checks whether it is performed correctly.  Unfortunately,
there is no easy fix for this problem, since the hypervisor has no indication
whether any iteration was executed.

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

diff --git a/x86/eventinj.c b/x86/eventinj.c
index 32de6f0..8fa4d84 100644
--- a/x86/eventinj.c
+++ b/x86/eventinj.c
@@ -54,6 +54,7 @@ static void flush_idt_page()
 
 static volatile unsigned int test_divider;
 static volatile int test_count;
+static volatile unsigned long rflags = 0;
 
 ulong stack_phys;
 void *stack_va;
@@ -190,6 +191,7 @@ static void tirq1(isr_regs_t *r)
 {
 	printf("irq1 running\n");
 	test_count++;
+	rflags = r->rflags;
 	eoi();
 }
 
@@ -208,6 +210,7 @@ int main()
 {
 	unsigned int res;
 	ulong *pt, *cr3, i;
+	unsigned char src[10], dst[10];
 
 	setup_vm();
 	setup_idt();
@@ -291,6 +294,19 @@ int main()
 	printf("After vec 32 and 33 to self\n");
 	report("vec 32/33", test_count == 2);
 
+	/* Inject HW interrupt on rep-movs and check RF */
+	test_count = 0;
+	flush_idt_page();
+	printf("Sending vec 33 to self\n");
+	apic_self_ipi(33);
+	io_delay();
+	irq_enable();
+	asm volatile("rep movsb\n" : :
+		"S"(src), "D"(dst), "c"(10) : "memory", "cc");
+	irq_disable();
+	printf("After vec 33 to self\n");
+	report("rflags.rf during rep movsb", test_count == 1 &&
+					     (rflags & (1<<16)));
 
 	/* Inject HW interrupt, do sti and than (while in irq shadow) inject
 	   soft interrupt. Fault during soft interrupt. Soft interrup shoud be
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/7] KVM: vmx: set rflags.rf during fault injection
  2014-07-21 11:37 ` [PATCH 4/7] KVM: vmx: set rflags.rf during fault injection Nadav Amit
@ 2014-07-21 12:05   ` Paolo Bonzini
  0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-21 12:05 UTC (permalink / raw)
  To: Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 13:37, Nadav Amit ha scritto:
> VMX does not automatically set rflags.rf during event injection. This patch
> does partial job, setting rflags.rf upon fault injection.  It also marks that
> injection of trap/interrupt during rep-string instruction is not properly
> emulated. It is unclear how to do it efficiently without decoding the guest
> instruction before interrupt injection.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>

Nothing in this patch is VMX-specific, right?

So it should be done in x86.c.

> ---
>  arch/x86/kvm/vmx.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 0c9569b..8edb785 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2006,6 +2006,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>  				bool reinject)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	unsigned long rflags;
>  	u32 intr_info = nr | INTR_INFO_VALID_MASK;
>  
>  	if (!reinject && is_guest_mode(vcpu) &&
> @@ -2017,6 +2018,12 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>  		intr_info |= INTR_INFO_DELIVER_CODE_MASK;
>  	}
>  
> +	rflags = vmx_get_rflags(vcpu);
> +	if (kvm_exception_type(nr) == EXCPT_FAULT)
> +		vmx_set_rflags(vcpu, rflags | X86_EFLAGS_RF);
> +
> +	/* TODO: Set rflags.rf on trap during rep-string */

For vmexits happening during a rep string operation that isn't emulated,
the processor should set RF correctly ("If the VM exit is caused
directly by an event that would normally be delivered through the IDT,
the value saved is that which would appear in the saved RFLAGS image had
the event been delivered through the IDT").

Perhaps the emulator could set RF to 1 after executing the first
iteration, and clear it after executing the last?

Paolo

> +
>  	if (vmx->rmode.vm86_active) {
>  		int inc_eip = 0;
>  		if (kvm_exception_is_soft(nr))
> @@ -4631,8 +4638,10 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
>  		intr |= INTR_TYPE_SOFT_INTR;
>  		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN,
>  			     vmx->vcpu.arch.event_exit_inst_len);
> -	} else
> +	} else {
> +		/* TODO: Set rflags.rf during rep-string */
>  		intr |= INTR_TYPE_EXT_INTR;
> +	}
>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr);
>  }
>  
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/7] KVM: x86: Function for determining exception type
  2014-07-21 11:37 ` [PATCH 2/7] KVM: x86: Function for determining exception type Nadav Amit
@ 2014-07-21 12:18   ` Paolo Bonzini
  2014-07-21 21:30     ` Nadav Amit
  0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-21 12:18 UTC (permalink / raw)
  To: Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 13:37, Nadav Amit ha scritto:
> +int kvm_exception_type(unsigned int nr)

The manual calls this the exception class.

Please open code this as an if like this

	int mask;

	/* This should never happen, right? */
	if (WARN_ON(nr > 31))
		return EXCPT_INTERRUPT;

	mask = 1 << nr;
	if (mask &
	    ((1 << DB_VECTOR) | (1 << BP_VECTOR) |
	     (1 << OF_VECTOR)))
		return EXCPT_TRAP;

	...

	/*
	 * If it is reserved, assume it is a fault and
	 * set RF.
	 */
	return EXCPT_FAULT;

> +	case VE_VECTOR:
> +		return EXCPT_FAULT;
> +	case DB_VECTOR:
> +		return EXCPT_FAULT_OR_TRAP;

It is only a fault for instruction fetch breakpoints.  You can modify
kvm_vcpu_check_breakpoint to set RF, add a comment here that fault
handling is done elsewhere, and return EXCPT_TRAP.

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/7] KVM: x86: Additional rflags.rf fixes
  2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
                   ` (7 preceding siblings ...)
  2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
@ 2014-07-21 12:19 ` Paolo Bonzini
  2014-07-21 12:28   ` Nadav Amit
  8 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-21 12:19 UTC (permalink / raw)
  To: Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 13:37, Nadav Amit ha scritto:
> RFLAGS.RF is not handled well by kvm, in both the x86 emulator and vmx code.
> This flag should be cleared after every instruction emulation (other than
> IRETD/IRETQ).  It should be set in various conditions as described in Intel SDM
> 17.3.1.1.  This series of patches addresses the clearing of RF on emulated
> instructions, the setting the RF upon fault injection.  It does not handle the
> case of traps and interrupts injection during REP-string, since there is
> no easy indication whether the first iteration of a rep-string occurred.
> The value of RF depends on whether the first iteration took place.
> 
> Thanks for reviewing the patches.
> 
> Nadav Amit (7):
>   KVM: x86: Defining missing x86 vectors
>   KVM: x86: Function for determining exception type
>   KVM: x86: Clearing rflags.rf upon skipped emulated instruction
>   KVM: vmx: set rflags.rf during fault injection
>   KVM: x86: popf emulation should not change RF
>   KVM: x86: Clear rflags.rf on emulated instructions
>   KVM: x86: Cleanup of rflags.rf cleaning
> 
>  arch/x86/include/uapi/asm/kvm.h |  3 +++
>  arch/x86/kvm/emulate.c          | 13 ++++++++-----
>  arch/x86/kvm/vmx.c              | 11 ++++++++++-
>  arch/x86/kvm/x86.c              | 37 +++++++++++++++++++++++++++++++++++++
>  arch/x86/kvm/x86.h              |  9 +++++++++
>  5 files changed, 67 insertions(+), 6 deletions(-)
> 

I'm not applying patches 2 and 4 yet.  I have applied the others:

      KVM: x86: Clearing rflags.rf upon skipped emulated instruction
      KVM: x86: popf emulation should not change RF
      KVM: x86: Clear rflags.rf on emulated instructions
      KVM: x86: Cleanup of rflags.rf cleaning
      KVM: x86: emulator injects #DB when RFLAGS.RF is set
      KVM: x86: Defining missing x86 vectors

since the remaining two are independent.

Thanks!

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH kvm-unit-tests 2/3] x86: Test rflags.rf is set upon faults
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 2/3] x86: Test rflags.rf is set upon faults Nadav Amit
@ 2014-07-21 12:24     ` Paolo Bonzini
  0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-21 12:24 UTC (permalink / raw)
  To: Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 13:39, Nadav Amit ha scritto:
> @@ -176,7 +176,7 @@ unsigned exception_vector(void)
>      unsigned short vector;
>  
>      asm("mov %%gs:4, %0" : "=rm"(vector));

"rm" is wrong here, it should be "r".  If we make it "q" instead, we can
use movb.


	unsigned char vector;

	asm("movb %%gs:4, %b0" : "=q"(vector));

> -    return vector;
> +    return (u8)vector;
>  }

> 
> +bool exception_rflags_rf(void)
> +{
> +    unsigned short rf_flag;
> +
> +    asm("mov %%gs:4, %0" : "=rm"(rf_flag));
> +    return (rf_flag >> 8) & 1;
> +}
> +

Same here, use "movb %%gs:5, %b0" and an unsigned char.

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting
  2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
                     ` (2 preceding siblings ...)
  2014-07-21 11:39   ` [PATCH kvm-unit-tests 3/3] x86: Check RFLAGS.RF on interrupt during REP-str Nadav Amit
@ 2014-07-21 12:25   ` Paolo Bonzini
  2014-07-24 11:55     ` [PATCH kvm-unit-tests] x86: Test rflags.rf is set upon faults Nadav Amit
  3 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-21 12:25 UTC (permalink / raw)
  To: Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 13:39, Nadav Amit ha scritto:
> This series of patches introduces checks for rflags.rf and whether it is
> cleared after emulation, and set correctly.  The last (third) patch fails even
> with recent fixes, since there is no easy way for the hypervisor to determine
> whether any iteration of rep-string was executed before. RFLAGS.RF should be
> cleared before the first iteration, and set otherwise.
> 
> Nadav Amit (3):
>   x86: Check rflags.rf is cleared after emulation
>   x86: Test rflags.rf is set upon faults
>   x86: Check RFLAGS.RF on interrupt during REP-str
> 
>  lib/x86/desc.c | 14 +++++++++++---
>  lib/x86/desc.h |  1 +
>  x86/eventinj.c | 16 ++++++++++++++++
>  x86/idt_test.c | 13 +++++++++----
>  x86/realmode.c |  3 ++-
>  5 files changed, 39 insertions(+), 8 deletions(-)
> 

I applied patch 1.

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/7] KVM: x86: Additional rflags.rf fixes
  2014-07-21 12:19 ` [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Paolo Bonzini
@ 2014-07-21 12:28   ` Nadav Amit
  2014-07-21 12:31     ` Paolo Bonzini
  0 siblings, 1 reply; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 12:28 UTC (permalink / raw)
  To: Paolo Bonzini, Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

On 7/21/14, 3:19 PM, Paolo Bonzini wrote:
> Il 21/07/2014 13:37, Nadav Amit ha scritto:
>> RFLAGS.RF is not handled well by kvm, in both the x86 emulator and vmx code.
>> This flag should be cleared after every instruction emulation (other than
>> IRETD/IRETQ).  It should be set in various conditions as described in Intel SDM
>> 17.3.1.1.  This series of patches addresses the clearing of RF on emulated
>> instructions, the setting the RF upon fault injection.  It does not handle the
>> case of traps and interrupts injection during REP-string, since there is
>> no easy indication whether the first iteration of a rep-string occurred.
>> The value of RF depends on whether the first iteration took place.
>>
>> Thanks for reviewing the patches.
>>
>> Nadav Amit (7):
>>    KVM: x86: Defining missing x86 vectors
>>    KVM: x86: Function for determining exception type
>>    KVM: x86: Clearing rflags.rf upon skipped emulated instruction
>>    KVM: vmx: set rflags.rf during fault injection
>>    KVM: x86: popf emulation should not change RF
>>    KVM: x86: Clear rflags.rf on emulated instructions
>>    KVM: x86: Cleanup of rflags.rf cleaning
>>
>>   arch/x86/include/uapi/asm/kvm.h |  3 +++
>>   arch/x86/kvm/emulate.c          | 13 ++++++++-----
>>   arch/x86/kvm/vmx.c              | 11 ++++++++++-
>>   arch/x86/kvm/x86.c              | 37 +++++++++++++++++++++++++++++++++++++
>>   arch/x86/kvm/x86.h              |  9 +++++++++
>>   5 files changed, 67 insertions(+), 6 deletions(-)
>>
>
> I'm not applying patches 2 and 4 yet.  I have applied the others:
>
>        KVM: x86: Clearing rflags.rf upon skipped emulated instruction
>        KVM: x86: popf emulation should not change RF
>        KVM: x86: Clear rflags.rf on emulated instructions
>        KVM: x86: Cleanup of rflags.rf cleaning
>        KVM: x86: emulator injects #DB when RFLAGS.RF is set
>        KVM: x86: Defining missing x86 vectors
>
> since the remaining two are independent.
>

Thanks for the quick response. I will address the issues you raised.

Please review and apply as well "[PATCH] KVM: x86: emulator injects #DB 
when RFLAGS.RF is set" which was submitted before. ( 
http://www.spinics.net/lists/kvm/msg105858.html ).
as well.

Thanks again,
Nadav


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/7] KVM: x86: Additional rflags.rf fixes
  2014-07-21 12:28   ` Nadav Amit
@ 2014-07-21 12:31     ` Paolo Bonzini
  2014-07-24 11:51       ` [PATCH 0/2] KVM: x86: Missing " Nadav Amit
  0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-21 12:31 UTC (permalink / raw)
  To: Nadav Amit, Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 14:28, Nadav Amit ha scritto:
>>
> 
> Thanks for the quick response. I will address the issues you raised.
> 
> Please review and apply as well "[PATCH] KVM: x86: emulator injects #DB
> when RFLAGS.RF is set" which was submitted before. (
> http://www.spinics.net/lists/kvm/msg105858.html ).
> as well.

That's the penultimate patch I applied:

>>        KVM: x86: Clearing rflags.rf upon skipped emulated instruction
>>        KVM: x86: popf emulation should not change RF
>>        KVM: x86: Clear rflags.rf on emulated instructions
>>        KVM: x86: Cleanup of rflags.rf cleaning
>>        KVM: x86: emulator injects #DB when RFLAGS.RF is set
>>        KVM: x86: Defining missing x86 vectors 

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/7] KVM: x86: Function for determining exception type
  2014-07-21 12:18   ` Paolo Bonzini
@ 2014-07-21 21:30     ` Nadav Amit
  2014-07-22  8:08       ` Paolo Bonzini
  0 siblings, 1 reply; 26+ messages in thread
From: Nadav Amit @ 2014-07-21 21:30 UTC (permalink / raw)
  To: Paolo Bonzini, Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Few comments to see we are on the same page:

On 7/21/14, 3:18 PM, Paolo Bonzini wrote:
> Il 21/07/2014 13:37, Nadav Amit ha scritto:
>> +int kvm_exception_type(unsigned int nr)
>
> The manual calls this the exception class.
Yes, but it also calls it exception "type" (see table 6-1 
"Protected-Mode Exceptions and Interrupts" on the SDM).
I called it exception type, since there is a function exception_class 
that is used to handle nested exceptions.

>> +	case VE_VECTOR:
>> +		return EXCPT_FAULT;
>> +	case DB_VECTOR:
>> +		return EXCPT_FAULT_OR_TRAP;
>
> It is only a fault for instruction fetch breakpoints.  You can modify
> kvm_vcpu_check_breakpoint to set RF, add a comment here that fault
> handling is done elsewhere, and return EXCPT_TRAP.
Unless I am mistaken, kvm_vcpu_check_breakpoint checks only for 
instruction breakpoint. Since instruction breakpoint should not cause RF 
to be set, this function should not be changed.

Anyhow, I would return EXCPT_TRAP on DB_VECTOR.

Nadav


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/7] KVM: x86: Function for determining exception type
  2014-07-21 21:30     ` Nadav Amit
@ 2014-07-22  8:08       ` Paolo Bonzini
  0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-22  8:08 UTC (permalink / raw)
  To: Nadav Amit, Nadav Amit; +Cc: tglx, mingo, hpa, x86, gleb, linux-kernel

Il 21/07/2014 23:30, Nadav Amit ha scritto:
> Few comments to see we are on the same page:
> 
> On 7/21/14, 3:18 PM, Paolo Bonzini wrote:
>> Il 21/07/2014 13:37, Nadav Amit ha scritto:
>>> +int kvm_exception_type(unsigned int nr)
>>
>> The manual calls this the exception class.
> Yes, but it also calls it exception "type" (see table 6-1
> "Protected-Mode Exceptions and Interrupts" on the SDM).
> I called it exception type, since there is a function exception_class
> that is used to handle nested exceptions.

Ok.

>>> +    case VE_VECTOR:
>>> +        return EXCPT_FAULT;
>>> +    case DB_VECTOR:
>>> +        return EXCPT_FAULT_OR_TRAP;
>>
>> It is only a fault for instruction fetch breakpoints.  You can modify
>> kvm_vcpu_check_breakpoint to set RF, add a comment here that fault
>> handling is done elsewhere, and return EXCPT_TRAP.
> Unless I am mistaken, kvm_vcpu_check_breakpoint checks only for
> instruction breakpoint. Since instruction breakpoint should not cause RF
> to be set, this function should not be changed.

You're right ("For any fault-class exception except a debug exception
generated in response to an instruction breakpoint, the
value pushed for RF is 1").  It's the debug exception handler that has
to set RF (and then iretd/iretq will modify RF).

> Anyhow, I would return EXCPT_TRAP on DB_VECTOR.

Yeah, just add a comment that it can be a fault for instruction fetch
breakpoints, but we treat it as a trap for the purposes of (not) setting RF.

Alternatively, just rename the function to exception_sets_rf and return
true/false.

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 0/2] KVM: x86: Missing rflags.rf fixes
  2014-07-21 12:31     ` Paolo Bonzini
@ 2014-07-24 11:51       ` Nadav Amit
  2014-07-24 11:51         ` [PATCH 1/2] KVM: x86: Setting rflags.rf during rep-string emulation Nadav Amit
  2014-07-24 11:51         ` [PATCH 2/2] KVM: x86: set rflags.rf during fault injection Nadav Amit
  0 siblings, 2 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-24 11:51 UTC (permalink / raw)
  To: pbonzini
  Cc: gleb, tglx, mingo, hpa, x86, linux-kernel, nadav.amit, Nadav Amit

These two patches address two remaining issues from the previous RF fixes
patch-set.  One is setting RF during rep-string emulation, so if the
instruction emulation is incomplete, the entry back to the guest will perfromed
with RF=1 so no instruction breakpoint would occur. The second sets RF when
injecting a fault to the guest.

Nadav Amit (2):
  KVM: x86: Setting rflags.rf during rep-string emulation
  KVM: x86: set rflags.rf during fault injection

 arch/x86/kvm/emulate.c |  6 +++++-
 arch/x86/kvm/x86.c     | 30 ++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/2] KVM: x86: Setting rflags.rf during rep-string emulation
  2014-07-24 11:51       ` [PATCH 0/2] KVM: x86: Missing " Nadav Amit
@ 2014-07-24 11:51         ` Nadav Amit
  2014-07-24 11:51         ` [PATCH 2/2] KVM: x86: set rflags.rf during fault injection Nadav Amit
  1 sibling, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-24 11:51 UTC (permalink / raw)
  To: pbonzini
  Cc: gleb, tglx, mingo, hpa, x86, linux-kernel, nadav.amit, Nadav Amit

This patch updates RF for rep-string emulation.  The flag is set upon the first
iteration, and cleared after the last (if emulated). It is intended to make
sure that if a trap (in future data/io #DB emulation) or interrupt is delivered
to the guest during the rep-string instruction, RF will be set correctly. RF
affects whether instruction breakpoint in the guest is masked.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8d41556..57743ed 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4683,7 +4683,10 @@ special_insn:
 			goto done;
 	}
 
-	ctxt->eflags &= ~EFLG_RF;
+	if (ctxt->rep_prefix && (ctxt->d & String))
+		ctxt->eflags |= EFLG_RF;
+	else
+		ctxt->eflags &= ~EFLG_RF;
 
 	if (ctxt->execute) {
 		if (ctxt->d & Fastop) {
@@ -4824,6 +4827,7 @@ writeback:
 			}
 			goto done; /* skip rip writeback */
 		}
+		ctxt->eflags &= ~EFLG_RF;
 	}
 
 	ctxt->eip = ctxt->_eip;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/2] KVM: x86: set rflags.rf during fault injection
  2014-07-24 11:51       ` [PATCH 0/2] KVM: x86: Missing " Nadav Amit
  2014-07-24 11:51         ` [PATCH 1/2] KVM: x86: Setting rflags.rf during rep-string emulation Nadav Amit
@ 2014-07-24 11:51         ` Nadav Amit
  1 sibling, 0 replies; 26+ messages in thread
From: Nadav Amit @ 2014-07-24 11:51 UTC (permalink / raw)
  To: pbonzini
  Cc: gleb, tglx, mingo, hpa, x86, linux-kernel, nadav.amit, Nadav Amit

x86 does not automatically set rflags.rf during event injection. This patch
does partial job, setting rflags.rf upon fault injection.  It does not handle
the setting of RF upon interrupt injection on rep-string instruction.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1fd806c..b24c518 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -311,6 +311,31 @@ static int exception_class(int vector)
 	return EXCPT_BENIGN;
 }
 
+#define EXCPT_FAULT		0
+#define EXCPT_TRAP		1
+#define EXCPT_ABORT		2
+#define EXCPT_INTERRUPT		3
+
+static int exception_type(int vector)
+{
+	unsigned int mask;
+
+	if (WARN_ON(vector > 31 || vector == NMI_VECTOR))
+		return EXCPT_INTERRUPT;
+
+	mask = 1 << vector;
+
+	/* #DB is trap, as instruction watchpoints are handled elsewhere */
+	if (mask & ((1 << DB_VECTOR) | (1 << BP_VECTOR) | (1 << OF_VECTOR)))
+		return EXCPT_TRAP;
+
+	if (mask & ((1 << DF_VECTOR) | (1 << MC_VECTOR)))
+		return EXCPT_ABORT;
+
+	/* Reserved exceptions will result in fault */
+	return EXCPT_FAULT;
+}
+
 static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
 		unsigned nr, bool has_error, u32 error_code,
 		bool reinject)
@@ -5882,6 +5907,11 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
 		trace_kvm_inj_exception(vcpu->arch.exception.nr,
 					vcpu->arch.exception.has_error_code,
 					vcpu->arch.exception.error_code);
+
+		if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
+			__kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
+					     X86_EFLAGS_RF);
+
 		kvm_x86_ops->queue_exception(vcpu, vcpu->arch.exception.nr,
 					  vcpu->arch.exception.has_error_code,
 					  vcpu->arch.exception.error_code,
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH kvm-unit-tests] x86: Test rflags.rf is set upon faults
  2014-07-21 12:25   ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Paolo Bonzini
@ 2014-07-24 11:55     ` Nadav Amit
  2014-07-24 12:09       ` Paolo Bonzini
  0 siblings, 1 reply; 26+ messages in thread
From: Nadav Amit @ 2014-07-24 11:55 UTC (permalink / raw)
  To: pbonzini
  Cc: gleb, tglx, mingo, hpa, x86, linux-kernel, nadav.amit, Nadav Amit

This patch tests whether rflags.rf is set upon #UD and #GP faults as it should,
according to Intel SDM 17.3.1.1.  The patch saves rflags.rf in an unused bit of
the value which is saved during exception handling to save rflags.rf.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 lib/x86/desc.c | 16 ++++++++++++----
 lib/x86/desc.h |  1 +
 x86/idt_test.c | 13 +++++++++----
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 9a80f48..7fbe774 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -36,8 +36,8 @@ static void check_exception_table(struct ex_regs *regs)
     struct ex_record *ex;
     unsigned ex_val;
 
-    ex_val = regs->vector | (regs->error_code << 16);
-
+    ex_val = regs->vector | (regs->error_code << 16) |
+		(((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));
 
     for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
@@ -173,9 +173,9 @@ void setup_idt(void)
 
 unsigned exception_vector(void)
 {
-    unsigned short vector;
+    unsigned char vector;
 
-    asm("mov %%gs:4, %0" : "=rm"(vector));
+    asm("movb %%gs:4, %0" : "=q"(vector));
     return vector;
 }
 
@@ -187,6 +187,14 @@ unsigned exception_error_code(void)
     return error_code;
 }
 
+bool exception_rflags_rf(void)
+{
+    unsigned char rf_flag;
+
+    asm("movb %%gs:5, %b0" : "=q"(rf_flag));
+    return rf_flag & 1;
+}
+
 static char intr_alt_stack[4096];
 
 #ifndef __x86_64__
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 553bce9..bd4293e 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -144,6 +144,7 @@ extern tss64_t tss;
 
 unsigned exception_vector(void);
 unsigned exception_error_code(void);
+bool exception_rflags_rf(void);
 void set_idt_entry(int vec, void *addr, int dpl);
 void set_idt_sel(int vec, u16 sel);
 void set_gdt_entry(int sel, u32 base,  u32 limit, u8 access, u8 gran);
diff --git a/x86/idt_test.c b/x86/idt_test.c
index ecb76bb..349aade 100644
--- a/x86/idt_test.c
+++ b/x86/idt_test.c
@@ -1,15 +1,16 @@
 #include "libcflat.h"
 #include "desc.h"
 
-int test_ud2(void)
+int test_ud2(bool *rflags_rf)
 {
     asm volatile(ASM_TRY("1f")
                  "ud2 \n\t"
                  "1:" :);
+    *rflags_rf = exception_rflags_rf();
     return exception_vector();
 }
 
-int test_gp(void)
+int test_gp(bool *rflags_rf)
 {
     unsigned long tmp;
 
@@ -18,19 +19,23 @@ int test_gp(void)
 		 "mov %0, %%cr4\n\t"
                  "1:"
                  : "=a"(tmp));
+    *rflags_rf = exception_rflags_rf();
     return exception_vector();
 }
 
 int main(void)
 {
     int r;
+    bool rflags_rf;
 
     printf("Starting IDT test\n");
     setup_idt();
-    r = test_gp();
+    r = test_gp(&rflags_rf);
     report("Testing #GP", r == GP_VECTOR);
-    r = test_ud2();
+    report("Testing #GP rflags.rf", rflags_rf);
+    r = test_ud2(&rflags_rf);
     report("Testing #UD", r == UD_VECTOR);
+    report("Testing #UD rflags.rf", rflags_rf);
 
     return report_summary();
 }
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH kvm-unit-tests] x86: Test rflags.rf is set upon faults
  2014-07-24 11:55     ` [PATCH kvm-unit-tests] x86: Test rflags.rf is set upon faults Nadav Amit
@ 2014-07-24 12:09       ` Paolo Bonzini
  0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2014-07-24 12:09 UTC (permalink / raw)
  To: Nadav Amit; +Cc: gleb, tglx, mingo, hpa, x86, linux-kernel, nadav.amit

Il 24/07/2014 13:55, Nadav Amit ha scritto:
> This patch tests whether rflags.rf is set upon #UD and #GP faults as it should,
> according to Intel SDM 17.3.1.1.  The patch saves rflags.rf in an unused bit of
> the value which is saved during exception handling to save rflags.rf.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  lib/x86/desc.c | 16 ++++++++++++----
>  lib/x86/desc.h |  1 +
>  x86/idt_test.c | 13 +++++++++----
>  3 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 9a80f48..7fbe774 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -36,8 +36,8 @@ static void check_exception_table(struct ex_regs *regs)
>      struct ex_record *ex;
>      unsigned ex_val;
>  
> -    ex_val = regs->vector | (regs->error_code << 16);
> -
> +    ex_val = regs->vector | (regs->error_code << 16) |
> +		(((regs->rflags >> 16) & 1) << 8);
>      asm("mov %0, %%gs:4" : : "r"(ex_val));
>  
>      for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
> @@ -173,9 +173,9 @@ void setup_idt(void)
>  
>  unsigned exception_vector(void)
>  {
> -    unsigned short vector;
> +    unsigned char vector;
>  
> -    asm("mov %%gs:4, %0" : "=rm"(vector));
> +    asm("movb %%gs:4, %0" : "=q"(vector));
>      return vector;
>  }
>  
> @@ -187,6 +187,14 @@ unsigned exception_error_code(void)
>      return error_code;
>  }
>  
> +bool exception_rflags_rf(void)
> +{
> +    unsigned char rf_flag;
> +
> +    asm("movb %%gs:5, %b0" : "=q"(rf_flag));
> +    return rf_flag & 1;
> +}
> +
>  static char intr_alt_stack[4096];
>  
>  #ifndef __x86_64__
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index 553bce9..bd4293e 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -144,6 +144,7 @@ extern tss64_t tss;
>  
>  unsigned exception_vector(void);
>  unsigned exception_error_code(void);
> +bool exception_rflags_rf(void);
>  void set_idt_entry(int vec, void *addr, int dpl);
>  void set_idt_sel(int vec, u16 sel);
>  void set_gdt_entry(int sel, u32 base,  u32 limit, u8 access, u8 gran);
> diff --git a/x86/idt_test.c b/x86/idt_test.c
> index ecb76bb..349aade 100644
> --- a/x86/idt_test.c
> +++ b/x86/idt_test.c
> @@ -1,15 +1,16 @@
>  #include "libcflat.h"
>  #include "desc.h"
>  
> -int test_ud2(void)
> +int test_ud2(bool *rflags_rf)
>  {
>      asm volatile(ASM_TRY("1f")
>                   "ud2 \n\t"
>                   "1:" :);
> +    *rflags_rf = exception_rflags_rf();
>      return exception_vector();
>  }
>  
> -int test_gp(void)
> +int test_gp(bool *rflags_rf)
>  {
>      unsigned long tmp;
>  
> @@ -18,19 +19,23 @@ int test_gp(void)
>  		 "mov %0, %%cr4\n\t"
>                   "1:"
>                   : "=a"(tmp));
> +    *rflags_rf = exception_rflags_rf();
>      return exception_vector();
>  }
>  
>  int main(void)
>  {
>      int r;
> +    bool rflags_rf;
>  
>      printf("Starting IDT test\n");
>      setup_idt();
> -    r = test_gp();
> +    r = test_gp(&rflags_rf);
>      report("Testing #GP", r == GP_VECTOR);
> -    r = test_ud2();
> +    report("Testing #GP rflags.rf", rflags_rf);
> +    r = test_ud2(&rflags_rf);
>      report("Testing #UD", r == UD_VECTOR);
> +    report("Testing #UD rflags.rf", rflags_rf);
>  
>      return report_summary();
>  }
> 

Thanks, applying to kvm-unit-tests.

Paolo

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2014-07-24 12:10 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-21 11:37 [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Nadav Amit
2014-07-21 11:37 ` [PATCH 1/7] KVM: x86: Defining missing x86 vectors Nadav Amit
2014-07-21 11:37 ` [PATCH 2/7] KVM: x86: Function for determining exception type Nadav Amit
2014-07-21 12:18   ` Paolo Bonzini
2014-07-21 21:30     ` Nadav Amit
2014-07-22  8:08       ` Paolo Bonzini
2014-07-21 11:37 ` [PATCH 3/7] KVM: x86: Clearing rflags.rf upon skipped emulated instruction Nadav Amit
2014-07-21 11:37 ` [PATCH 4/7] KVM: vmx: set rflags.rf during fault injection Nadav Amit
2014-07-21 12:05   ` Paolo Bonzini
2014-07-21 11:37 ` [PATCH 5/7] KVM: x86: popf emulation should not change RF Nadav Amit
2014-07-21 11:37 ` [PATCH 6/7] KVM: x86: Clear rflags.rf on emulated instructions Nadav Amit
2014-07-21 11:37 ` [PATCH 7/7] KVM: x86: Cleanup of rflags.rf cleaning Nadav Amit
2014-07-21 11:39 ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Nadav Amit
2014-07-21 11:39   ` [PATCH kvm-unit-tests 1/3] x86: Check rflags.rf is cleared after emulation Nadav Amit
2014-07-21 11:39   ` [PATCH kvm-unit-tests 2/3] x86: Test rflags.rf is set upon faults Nadav Amit
2014-07-21 12:24     ` Paolo Bonzini
2014-07-21 11:39   ` [PATCH kvm-unit-tests 3/3] x86: Check RFLAGS.RF on interrupt during REP-str Nadav Amit
2014-07-21 12:25   ` [PATCH kvm-unit-tests 0/3] x86: Test rflags.rf clearing/setting Paolo Bonzini
2014-07-24 11:55     ` [PATCH kvm-unit-tests] x86: Test rflags.rf is set upon faults Nadav Amit
2014-07-24 12:09       ` Paolo Bonzini
2014-07-21 12:19 ` [PATCH 0/7] KVM: x86: Additional rflags.rf fixes Paolo Bonzini
2014-07-21 12:28   ` Nadav Amit
2014-07-21 12:31     ` Paolo Bonzini
2014-07-24 11:51       ` [PATCH 0/2] KVM: x86: Missing " Nadav Amit
2014-07-24 11:51         ` [PATCH 1/2] KVM: x86: Setting rflags.rf during rep-string emulation Nadav Amit
2014-07-24 11:51         ` [PATCH 2/2] KVM: x86: set rflags.rf during fault injection Nadav Amit

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.