2007-10-21 11:08:23

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 00/11] KVM updates for Linux 2.6.24-rc review

The following patches fix fallout from the main 2.6.24 KVM merge. An
exception is the movnti emulation patch, which adds support for Linux
2.6.16 guests.

The patches can be found in kvm.git in the branch kvm-updates-2.6.24.
There is also a branch kvm-updates-2.6.25 which will form the basis of
the next merge window submission.

Please review the patches and let me know if something is wrong or if
a patch is missing.


Aurelien Jarno (1):
KVM: x86 emulator: fix access registers for instructions with ModR/M byte and Mod = 3

Avi Kivity (2):
KVM: VMX: Handle NMIs before enabling interrupts and preemption
KVM: VMX: Force vm86 mode if setting flags during real mode

Eddie Dong (1):
KVM: VMX: Reset mmu context when entering real mode

Izik Eidus (1):
KVM: MMU: Set shadow pte atomically in mmu_pte_write_zap_pte()

Kevin Pedretti (2):
KVM: Fix local apic timer divide by zero
KVM: Improve local apic timer wraparound handling

Laurent Vivier (2):
KVM: x86 emulator: fix repne/repnz decoding
KVM: Move kvm_guest_exit() after local_irq_enable()

Nitin A Kamble (1):
KVM: x86 emulator: fix merge screwup due to emulator split

Sheng Yang (1):
KVM: x86 emulator: implement 'movnti mem, reg'

drivers/kvm/kvm_main.c | 11 ++++++-
drivers/kvm/lapic.c | 38 ++++++++++++++++------
drivers/kvm/mmu.c | 3 +-
drivers/kvm/vmx.c | 16 +++++++--
drivers/kvm/x86_emulate.c | 77 ++++++++++++++++++++++++++++----------------
5 files changed, 100 insertions(+), 45 deletions(-)


2007-10-21 11:08:07

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 04/11] KVM: VMX: Handle NMIs before enabling interrupts and preemption

This makes sure we handle NMI on the current cpu, and that we don't service
maskable interrupts before non-maskable ones.

Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/vmx.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index 4f115a8..bcc1e39 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -1760,10 +1760,8 @@ static int handle_exception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
set_bit(irq / BITS_PER_LONG, &vcpu->irq_summary);
}

- if ((intr_info & INTR_INFO_INTR_TYPE_MASK) == 0x200) { /* nmi */
- asm ("int $2");
- return 1;
- }
+ if ((intr_info & INTR_INFO_INTR_TYPE_MASK) == 0x200) /* nmi */
+ return 1; /* already handled by vmx_vcpu_run() */

if (is_no_device(intr_info)) {
vmx_fpu_activate(vcpu);
@@ -2196,6 +2194,7 @@ static void vmx_intr_assist(struct kvm_vcpu *vcpu)
static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
+ u32 intr_info;

/*
* Loading guest fpu may have cleared host cr0.ts
@@ -2322,6 +2321,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)

asm ("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));
vmx->launched = 1;
+
+ intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+
+ /* We need to handle NMIs before interrupts are enabled */
+ if ((intr_info & INTR_INFO_INTR_TYPE_MASK) == 0x200) /* nmi */
+ asm("int $2");
}

static void vmx_inject_page_fault(struct kvm_vcpu *vcpu,
--
1.5.3

2007-10-21 11:08:36

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 01/11] KVM: x86 emulator: fix merge screwup due to emulator split

From: Nitin A Kamble <[email protected]>

This code has gone to wrong place in the file. Moving it back to
right location.

Signed-off-by: Nitin A Kamble <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/x86_emulate.c | 51 +++++++++++++++++++++++----------------------
1 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 9737c3b..b1026d2 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -1083,31 +1083,6 @@ push:
case 0xd2 ... 0xd3: /* Grp2 */
src.val = _regs[VCPU_REGS_RCX];
goto grp2;
- case 0xe8: /* call (near) */ {
- long int rel;
- switch (op_bytes) {
- case 2:
- rel = insn_fetch(s16, 2, _eip);
- break;
- case 4:
- rel = insn_fetch(s32, 4, _eip);
- break;
- case 8:
- rel = insn_fetch(s64, 8, _eip);
- break;
- default:
- DPRINTF("Call: Invalid op_bytes\n");
- goto cannot_emulate;
- }
- src.val = (unsigned long) _eip;
- JMP_REL(rel);
- goto push;
- }
- case 0xe9: /* jmp rel */
- case 0xeb: /* jmp rel short */
- JMP_REL(src.val);
- no_wb = 1; /* Disable writeback. */
- break;
case 0xf6 ... 0xf7: /* Grp3 */
switch (modrm_reg) {
case 0 ... 1: /* test */
@@ -1350,6 +1325,32 @@ special_insn:
case 0xae ... 0xaf: /* scas */
DPRINTF("Urk! I don't handle SCAS.\n");
goto cannot_emulate;
+ case 0xe8: /* call (near) */ {
+ long int rel;
+ switch (op_bytes) {
+ case 2:
+ rel = insn_fetch(s16, 2, _eip);
+ break;
+ case 4:
+ rel = insn_fetch(s32, 4, _eip);
+ break;
+ case 8:
+ rel = insn_fetch(s64, 8, _eip);
+ break;
+ default:
+ DPRINTF("Call: Invalid op_bytes\n");
+ goto cannot_emulate;
+ }
+ src.val = (unsigned long) _eip;
+ JMP_REL(rel);
+ goto push;
+ }
+ case 0xe9: /* jmp rel */
+ case 0xeb: /* jmp rel short */
+ JMP_REL(src.val);
+ no_wb = 1; /* Disable writeback. */
+ break;
+

}
goto writeback;
--
1.5.3

2007-10-21 11:08:49

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 02/11] KVM: x86 emulator: fix repne/repnz decoding

From: Laurent Vivier <[email protected]>

The repnz/repne instructions must set rep_prefix to 1 like rep/repe/repz.

This patch correct the disk probe problem met with OpenBSD.

This issue appears with commit e70669abd4e60dfea3ac1639848e20e2b8dd1255
because before it, the decoding was done internally to kvm and after it
is done by x86_emulate.c (which doesn't do it correctly).

Signed-off-by: Laurent Vivier <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/x86_emulate.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index b1026d2..80b1758 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -596,11 +596,10 @@ x86_emulate_memop(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
case 0xf0: /* LOCK */
lock_prefix = 1;
break;
+ case 0xf2: /* REPNE/REPNZ */
case 0xf3: /* REP/REPE/REPZ */
rep_prefix = 1;
break;
- case 0xf2: /* REPNE/REPNZ */
- break;
default:
goto done_prefixes;
}
--
1.5.3

2007-10-21 11:09:03

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 03/11] KVM: MMU: Set shadow pte atomically in mmu_pte_write_zap_pte()

From: Izik Eidus <[email protected]>

Setting shadow page table entry should be set atomicly using set_shadow_pte().

Signed-off-by: Izik Eidus <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/mmu.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index 6d84d30..7171618 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -1088,7 +1088,7 @@ static void mmu_pte_write_zap_pte(struct kvm_vcpu *vcpu,
mmu_page_remove_parent_pte(child, spte);
}
}
- *spte = 0;
+ set_shadow_pte(spte, 0);
kvm_flush_remote_tlbs(vcpu->kvm);
}

--
1.5.3

2007-10-21 11:09:32

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 05/11] KVM: VMX: Reset mmu context when entering real mode

From: Eddie Dong <[email protected]>

Resetting an SMP guest will force AP enter real mode (RESET) with
paging enabled in protected mode. While current enter_rmode() can
only handle mode switch from nonpaging mode to real mode which leads
to SMP reboot failure.

Fix by reloading the mmu context on entering real mode.

Signed-off-by: Yaozu (Eddie) Dong <[email protected]>
Signed-off-by: Qing He <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/mmu.c | 1 +
drivers/kvm/vmx.c | 1 +
2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index 7171618..feb5ac9 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -1049,6 +1049,7 @@ int kvm_mmu_reset_context(struct kvm_vcpu *vcpu)
destroy_kvm_mmu(vcpu);
return init_kvm_mmu(vcpu);
}
+EXPORT_SYMBOL_GPL(kvm_mmu_reset_context);

int kvm_mmu_load(struct kvm_vcpu *vcpu)
{
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index bcc1e39..f130c01 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -1128,6 +1128,7 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
fix_rmode_seg(VCPU_SREG_GS, &vcpu->rmode.gs);
fix_rmode_seg(VCPU_SREG_FS, &vcpu->rmode.fs);

+ kvm_mmu_reset_context(vcpu);
init_rmode_tss(vcpu->kvm);
}

--
1.5.3

2007-10-21 11:09:49

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 06/11] KVM: x86 emulator: implement 'movnti mem, reg'

From: Sheng Yang <[email protected]>

Implement emulation of instruction:
movnti m32/m64, r32/r64
opcode: 0x0f 0xc3

Signed-off-by: Sheng Yang <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/x86_emulate.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 80b1758..0a8696d 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -212,7 +212,8 @@ static u16 twobyte_table[256] = {
0, 0, ByteOp | DstReg | SrcMem | ModRM | Mov,
DstReg | SrcMem16 | ModRM | Mov,
/* 0xC0 - 0xCF */
- 0, 0, 0, 0, 0, 0, 0, ImplicitOps | ModRM, 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, DstMem | SrcReg | ModRM | Mov, 0, 0, 0, ImplicitOps | ModRM,
+ 0, 0, 0, 0, 0, 0, 0, 0,
/* 0xD0 - 0xDF */
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
/* 0xE0 - 0xEF */
@@ -1501,6 +1502,10 @@ twobyte_insn:
dst.bytes = op_bytes;
dst.val = (d & ByteOp) ? (s8) src.val : (s16) src.val;
break;
+ case 0xc3: /* movnti */
+ dst.bytes = op_bytes;
+ dst.val = (op_bytes == 4) ? (u32) src.val : (u64) src.val;
+ break;
}
goto writeback;

--
1.5.3

2007-10-21 11:10:06

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 08/11] KVM: x86 emulator: fix access registers for instructions with ModR/M byte and Mod = 3

From: Aurelien Jarno <[email protected]>

The patch belows changes the access type to register from memory for
instructions that are declared as SrcMem or DstMem, but have a
ModR/M byte with Mod = 3.

It fixes (at least) the lmsw and smsw instructions on an AMD64 CPU,
which are needed for FreeBSD.

Signed-off-by: Aurelien Jarno <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/x86_emulate.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 0a8696d..a6ace30 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -825,6 +825,14 @@ done_prefixes:
if (twobyte && b == 0x01 && modrm_reg == 7)
break;
srcmem_common:
+ /*
+ * For instructions with a ModR/M byte, switch to register
+ * access if Mod = 3.
+ */
+ if ((d & ModRM) && modrm_mod == 3) {
+ src.type = OP_REG;
+ break;
+ }
src.type = OP_MEM;
src.ptr = (unsigned long *)cr2;
src.val = 0;
@@ -893,6 +901,14 @@ done_prefixes:
dst.ptr = (unsigned long *)cr2;
dst.bytes = (d & ByteOp) ? 1 : op_bytes;
dst.val = 0;
+ /*
+ * For instructions with a ModR/M byte, switch to register
+ * access if Mod = 3.
+ */
+ if ((d & ModRM) && modrm_mod == 3) {
+ dst.type = OP_REG;
+ break;
+ }
if (d & BitOp) {
unsigned long mask = ~(dst.bytes * 8 - 1);

--
1.5.3

2007-10-21 11:10:38

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 07/11] KVM: VMX: Force vm86 mode if setting flags during real mode

When resetting from userspace, we need to handle the flags being cleared
even after we are in real mode.

Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/vmx.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index f130c01..bb56ae3 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -523,6 +523,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)

static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
{
+ if (vcpu->rmode.active)
+ rflags |= IOPL_MASK | X86_EFLAGS_VM;
vmcs_writel(GUEST_RFLAGS, rflags);
}

--
1.5.3

2007-10-21 11:10:51

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 09/11] KVM: Move kvm_guest_exit() after local_irq_enable()

From: Laurent Vivier <[email protected]>

We need to make sure that the timer interrupt happens before we clear
PF_VCPU, so the accounting code actually sees guest mode.

http://lkml.org/lkml/2007/10/15/114

Signed-off-by: Laurent Vivier <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/kvm_main.c | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index af2d288..8c458f2 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2054,12 +2054,21 @@ again:

kvm_x86_ops->run(vcpu, kvm_run);

- kvm_guest_exit();
vcpu->guest_mode = 0;
local_irq_enable();

++vcpu->stat.exits;

+ /*
+ * We must have an instruction between local_irq_enable() and
+ * kvm_guest_exit(), so the timer interrupt isn't delayed by
+ * the interrupt shadow. The stat.exits increment will do nicely.
+ * But we need to prevent reordering, hence this barrier():
+ */
+ barrier();
+
+ kvm_guest_exit();
+
preempt_enable();

/*
--
1.5.3

2007-10-21 11:11:13

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 10/11] KVM: Fix local apic timer divide by zero

From: Kevin Pedretti <[email protected]>

kvm_lapic_reset() was initializing apic->timer.divide_count to 0,
which could potentially lead to a divide by zero error in
apic_get_tmcct(). Any guest that reads the APIC's CCR (current count)
register before setting DCR (divide configuration) would trigger a divide
by zero exception in the host kernel, leading to a host-OS crash.

This patch results in apic->timer.divide_count being initialized to
2 at reset, eliminating the bug (DCR=0 at reset, meaning divide by 2).

Signed-off-by: Kevin Pedretti <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/lapic.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index a190587..443730e 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -853,7 +853,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
apic_set_reg(apic, APIC_ISR + 0x10 * i, 0);
apic_set_reg(apic, APIC_TMR + 0x10 * i, 0);
}
- apic->timer.divide_count = 0;
+ update_divide_count(apic);
atomic_set(&apic->timer.pending, 0);
if (vcpu->vcpu_id == 0)
vcpu->apic_base |= MSR_IA32_APICBASE_BSP;
--
1.5.3

2007-10-21 11:11:36

by Avi Kivity

[permalink] [raw]
Subject: [PATCH 11/11] KVM: Improve local apic timer wraparound handling

From: Kevin Pedretti <[email protected]>

Better handle wrap-around cases when reading the APIC CCR
(current count register). Also, if ICR is 0, CCR should also
be 0... previously reading CCR before setting ICR would result
in a large kinda-random number.

Signed-off-by: Kevin Pedretti <[email protected]>
Signed-off-by: Avi Kivity <[email protected]>
---
drivers/kvm/lapic.c | 36 ++++++++++++++++++++++++++----------
1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 443730e..238fcad 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -494,12 +494,19 @@ static void apic_send_ipi(struct kvm_lapic *apic)

static u32 apic_get_tmcct(struct kvm_lapic *apic)
{
- u32 counter_passed;
- ktime_t passed, now = apic->timer.dev.base->get_time();
- u32 tmcct = apic_get_reg(apic, APIC_TMICT);
+ u64 counter_passed;
+ ktime_t passed, now;
+ u32 tmcct;

ASSERT(apic != NULL);

+ now = apic->timer.dev.base->get_time();
+ tmcct = apic_get_reg(apic, APIC_TMICT);
+
+ /* if initial count is 0, current count should also be 0 */
+ if (tmcct == 0)
+ return 0;
+
if (unlikely(ktime_to_ns(now) <=
ktime_to_ns(apic->timer.last_update))) {
/* Wrap around */
@@ -514,15 +521,24 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)

counter_passed = div64_64(ktime_to_ns(passed),
(APIC_BUS_CYCLE_NS * apic->timer.divide_count));
- tmcct -= counter_passed;

- if (tmcct <= 0) {
- if (unlikely(!apic_lvtt_period(apic)))
+ if (counter_passed > tmcct) {
+ if (unlikely(!apic_lvtt_period(apic))) {
+ /* one-shot timers stick at 0 until reset */
tmcct = 0;
- else
- do {
- tmcct += apic_get_reg(apic, APIC_TMICT);
- } while (tmcct <= 0);
+ } else {
+ /*
+ * periodic timers reset to APIC_TMICT when they
+ * hit 0. The while loop simulates this happening N
+ * times. (counter_passed %= tmcct) would also work,
+ * but might be slower or not work on 32-bit??
+ */
+ while (counter_passed > tmcct)
+ counter_passed -= tmcct;
+ tmcct -= counter_passed;
+ }
+ } else {
+ tmcct -= counter_passed;
}

return tmcct;
--
1.5.3

2007-10-21 11:13:11

by Avi Kivity

[permalink] [raw]
Subject: Re: [PATCH 00/11] KVM updates for Linux 2.6.24-rc review

Avi Kivity wrote:
> The following patches fix fallout from the main 2.6.24 KVM merge. An
> exception is the movnti emulation patch, which adds support for Linux
> 2.6.16 guests.
>
> The patches can be found in kvm.git in the branch kvm-updates-2.6.24.
> There is also a branch kvm-updates-2.6.25 which will form the basis of
> the next merge window submission.
>
> Please review the patches and let me know if something is wrong or if
> a patch is missing.
>
>


Laurent, I believe the following patch (in kvm-updates-2.6.25) needs to
go into 2.6.24 as well. Can you comment on this?

> commit 6de232e39be372f85bea96eb741962acc7fcb1f7
> Author: Laurent Vivier <[email protected]>
> Date: Mon Oct 1 11:01:06 2007 +0200
>
> KVM: x86 emulator: Correct management of REP prefix
>
> This patch corrects some errors appearing when we have an
> emulation failure
> on an operation using REP prefix.
>
> When x86_emulate_insn() fails, saving EIP and ECX is not enough as
> emulation
> should have modified other registers like RSI or RDI. Moreover,
> the emulation
> can fail on the writeback, and in this case we are not able to restore
> registers.
>
> At beginning of x86_emulate_insn(), we restore registers from vcpu
> as they were
> not modified by x86d_decode_insn() and we save EIP to be able to
> restore it
> in case of failure.
>

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

2007-10-21 11:54:39

by Laurent Vivier

[permalink] [raw]
Subject: Re: [PATCH 00/11] KVM updates for Linux 2.6.24-rc review

Avi Kivity a ?crit :
> Avi Kivity wrote:
>> The following patches fix fallout from the main 2.6.24 KVM merge. An
>> exception is the movnti emulation patch, which adds support for Linux
>> 2.6.16 guests.
>>
>> The patches can be found in kvm.git in the branch kvm-updates-2.6.24.
>> There is also a branch kvm-updates-2.6.25 which will form the basis of
>> the next merge window submission.
>>
>> Please review the patches and let me know if something is wrong or if
>> a patch is missing.
>>
>>
>
>
> Laurent, I believe the following patch (in kvm-updates-2.6.25) needs to
> go into 2.6.24 as well. Can you comment on this?

What kind of comment do you want ?
What are the requirements to go in 2.6.24 instead of 2.6.25 ?

Is a bug correction enough ? :-P

This patch correct a bad behavior of x86_emulate_insn() in case of error with a
REP prefix.

This patch is needed because, without it, when REP prefix is used with an
instruction failing for some reasons (like IO or page fault) we don't restore
all modified registers (like RSI and RDI), but only ECX and EIP, so when we
re-enter in x86_emulate_insn() we modify again an already modified value.
Moreover, this patch manages correctly the case where the instruction fails in
writeback().


>> commit 6de232e39be372f85bea96eb741962acc7fcb1f7
>> Author: Laurent Vivier <[email protected]>
>> Date: Mon Oct 1 11:01:06 2007 +0200
>>
>> KVM: x86 emulator: Correct management of REP prefix
>>
>> This patch corrects some errors appearing when we have an
>> emulation failure
>> on an operation using REP prefix.
>>
>> When x86_emulate_insn() fails, saving EIP and ECX is not enough as
>> emulation
>> should have modified other registers like RSI or RDI. Moreover,
>> the emulation
>> can fail on the writeback, and in this case we are not able to
>> restore
>> registers.
>>
>> At beginning of x86_emulate_insn(), we restore registers from vcpu
>> as they were
>> not modified by x86d_decode_insn() and we save EIP to be able to
>> restore it
>> in case of failure.
>>
>


--
---------------- [email protected] -----------------
"Given enough eyeballs, all bugs are shallow" E. S. Raymond

2007-10-21 12:07:52

by Avi Kivity

[permalink] [raw]
Subject: Re: [PATCH 00/11] KVM updates for Linux 2.6.24-rc review

Laurent Vivier wrote:
> Avi Kivity a ?crit :
>> Avi Kivity wrote:
>>> The following patches fix fallout from the main 2.6.24 KVM merge. An
>>> exception is the movnti emulation patch, which adds support for Linux
>>> 2.6.16 guests.
>>>
>>> The patches can be found in kvm.git in the branch kvm-updates-2.6.24.
>>> There is also a branch kvm-updates-2.6.25 which will form the basis of
>>> the next merge window submission.
>>>
>>> Please review the patches and let me know if something is wrong or if
>>> a patch is missing.
>>>
>>>
>>
>>
>> Laurent, I believe the following patch (in kvm-updates-2.6.25) needs
>> to go into 2.6.24 as well. Can you comment on this?
>
> What kind of comment do you want ?

Well, whether it needs to go into .24.

> What are the requirements to go in 2.6.24 instead of 2.6.25 ?
>
> Is a bug correction enough ? :-P
>

It has to fix something real-life, in a real guest.

> This patch correct a bad behavior of x86_emulate_insn() in case of
> error with a REP prefix.
>
> This patch is needed because, without it, when REP prefix is used with
> an instruction failing for some reasons (like IO or page fault) we
> don't restore all modified registers (like RSI and RDI), but only ECX
> and EIP, so when we re-enter in x86_emulate_insn() we modify again an
> already modified value. Moreover, this patch manages correctly the
> case where the instruction fails in writeback().
>

Okay, I guess it is needed. Can you backport it to the branch? It's
very different from kvm-updates-2.6.24...

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

2007-10-21 12:39:09

by Laurent Vivier

[permalink] [raw]
Subject: Re: [kvm-devel] [PATCH 00/11] KVM updates for Linux 2.6.24-rc review

Avi Kivity a ?crit :
> Laurent Vivier wrote:
>> Avi Kivity a ?crit :
>>> Avi Kivity wrote:
>>>> The following patches fix fallout from the main 2.6.24 KVM merge. An
>>>> exception is the movnti emulation patch, which adds support for Linux
>>>> 2.6.16 guests.
>>>>
>>>> The patches can be found in kvm.git in the branch kvm-updates-2.6.24.
>>>> There is also a branch kvm-updates-2.6.25 which will form the basis of
>>>> the next merge window submission.
>>>>
>>>> Please review the patches and let me know if something is wrong or if
>>>> a patch is missing.
>>>>
>>>>
>>>
>>> Laurent, I believe the following patch (in kvm-updates-2.6.25) needs
>>> to go into 2.6.24 as well. Can you comment on this?
>> What kind of comment do you want ?
>
> Well, whether it needs to go into .24.
>
>> What are the requirements to go in 2.6.24 instead of 2.6.25 ?
>>
>> Is a bug correction enough ? :-P
>>
>
> It has to fix something real-life, in a real guest.
>
>> This patch correct a bad behavior of x86_emulate_insn() in case of
>> error with a REP prefix.
>>
>> This patch is needed because, without it, when REP prefix is used with
>> an instruction failing for some reasons (like IO or page fault) we
>> don't restore all modified registers (like RSI and RDI), but only ECX
>> and EIP, so when we re-enter in x86_emulate_insn() we modify again an
>> already modified value. Moreover, this patch manages correctly the
>> case where the instruction fails in writeback().
>>
>
> Okay, I guess it is needed. Can you backport it to the branch? It's
> very different from kvm-updates-2.6.24...
>

Well, in fact it is not needed in 2.6.24, because this patch correct a bad
behavior introduced by commit 57f4e446ebca4aad5c11364baf8477c8cfcb16a4 (which is
not in kvm-update-2.6.24):

KVM: Call x86_decode_insn() only when needed

Move emulate_ctxt to kvm_vcpu to keep emulate context when we exit from kvm
module. Call x86_decode_insn() only when needed. Modify x86_emulate_insn() to
not modify the context if it must be re-entered.

So, in fact, the answer is (after correctly understanding the question): no,
this patch is not needed in kvm-update-2.6.24.

Regards,
Laurent
--
---------------- [email protected] -----------------
"Given enough eyeballs, all bugs are shallow" E. S. Raymond

2007-10-22 09:36:57

by Avi Kivity

[permalink] [raw]
Subject: Re: [kvm-devel] [PATCH 00/11] KVM updates for Linux 2.6.24-rc review

Laurent Vivier wrote:
>
> Well, in fact it is not needed in 2.6.24, because this patch correct a
> bad behavior introduced by commit
> 57f4e446ebca4aad5c11364baf8477c8cfcb16a4 (which is not in
> kvm-update-2.6.24):
>
> KVM: Call x86_decode_insn() only when needed
>
> Move emulate_ctxt to kvm_vcpu to keep emulate context when we exit
> from kvm
> module. Call x86_decode_insn() only when needed. Modify
> x86_emulate_insn() to
> not modify the context if it must be re-entered.
>
> So, in fact, the answer is (after correctly understanding the
> question): no, this patch is not needed in kvm-update-2.6.24.



Thanks. I've folded it into 57f4e446ebca4aad5c11364baf8477c8cfcb16a4
for 2.6.25.

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