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
When skipping an emulated instruction, rflags.rf should be cleared as it would
be on real x86 CPU.
Signed-off-by: Nadav Amit <[email protected]>
---
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
Defining XE, XM and VE vector numbers.
Signed-off-by: Nadav Amit <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
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);
> }
>
>
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
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
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
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
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
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
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
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
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 <[email protected]>
---
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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