Skip to contentSkip to conversation

nVMX: Shadowing of CPU_BASED_VM_EXEC_CONTROL broken

DISCUSSION Jan Kiszka 3 participants 13 messages Oct 8, 2014 – Oct 9, 2014

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

Hi all,

after migrating a Jailhouse VM to a newer host platform with shadow VMCS support I found a bug. As you may know, Jailhouse doesn't intercept interrupts, thus also never requests an interrupt window. Nevertheless:

 qemu-system-x86-5777  [001] 74970.625324: kvm_mmio:             mmio write len 4 gpa 0xfebf5008 val 0x20 qemu-system-x86-5777  [001] 74970.625325: kvm_userspace_exit:   reason KVM_EXIT_MMIO (6)
 qemu-system-x86-5777  [001] 74970.625330: kvm_entry:            vcpu 1
 qemu-system-x86-5777  [001] 74970.625333: kvm_exit:             reason PENDING_INTERRUPT rip 0xffffffff81043e54 info 0 0
 qemu-system-x86-5777  [001] 74970.625333: kvm_nested_vmexit:    rip: 0xffffffff81043e54 reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
 qemu-system-x86-5777  [001] 74970.625334: kvm_nested_vmexit_inject: reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
 qemu-system-x86-5777  [001] 74970.625339: kvm_entry:            vcpu 1
 qemu-system-x86-5777  [001] 74970.625341: kvm_exit:             reason EPT_MISCONFIG rip 0xfffffffff0002307 info 0 0
 qemu-system-x86-5777  [001] 74970.625343: kvm_emulate_insn:     0:fffffffff0002307:8b 40 20 (prot64)

And then Jailhouse crashes (which is also interesting to understand why
- L1 host state is corrupt). Anyway, the point is that we leak CPU_BASED_VIRTUAL_INTR_PENDING from L0 into vmcs12. L0 sets it before entering L2, and then we transfer it from the hardware state to vmc12 on exit because that VMCS field is shadowed. The crash disappears when disabling VMCS shadowing.

Can we simply stop shadowing CPU_BASED_VM_EXEC_CONTROL when CPU_BASED_VIRTUAL_INTR_PENDING is injected to L2?

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
maintainer
#1 lore raw

Il 08/10/2014 10:29, Jan Kiszka ha scritto:

Quoted 22 lines

Hi all,

after migrating a Jailhouse VM to a newer host platform with shadow VMCS support I found a bug. As you may know, Jailhouse doesn't intercept interrupts, thus also never requests an interrupt window. Nevertheless:

 qemu-system-x86-5777  [001] 74970.625324: kvm_mmio:             mmio write len 4 gpa 0xfebf5008 val 0x20 qemu-system-x86-5777  [001] 74970.625325: kvm_userspace_exit:   reason KVM_EXIT_MMIO (6)
 qemu-system-x86-5777  [001] 74970.625330: kvm_entry:            vcpu 1
 qemu-system-x86-5777  [001] 74970.625333: kvm_exit:             reason PENDING_INTERRUPT rip 0xffffffff81043e54 info 0 0
 qemu-system-x86-5777  [001] 74970.625333: kvm_nested_vmexit:    rip: 0xffffffff81043e54 reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
 qemu-system-x86-5777  [001] 74970.625334: kvm_nested_vmexit_inject: reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
 qemu-system-x86-5777  [001] 74970.625339: kvm_entry:            vcpu 1
 qemu-system-x86-5777  [001] 74970.625341: kvm_exit:             reason EPT_MISCONFIG rip 0xfffffffff0002307 info 0 0
 qemu-system-x86-5777  [001] 74970.625343: kvm_emulate_insn:     0:fffffffff0002307:8b 40 20 (prot64)

And then Jailhouse crashes (which is also interesting to understand why
- L1 host state is corrupt). Anyway, the point is that we leak CPU_BASED_VIRTUAL_INTR_PENDING from L0 into vmcs12. L0 sets it before entering L2, and then we transfer it from the hardware state to vmc12 on exit because that VMCS field is shadowed. The crash disappears when disabling VMCS shadowing.

Quoted 2 lines

Can we simply stop shadowing CPU_BASED_VM_EXEC_CONTROL when CPU_BASED_VIRTUAL_INTR_PENDING is injected to L2?

The main problem is that we have a single shadowing bitmap for all virtual machines. I cannot think of a simple solution except not shadowing that field at all.

Paolo

author
#2 lore raw
Quoted 23 lines · Paolo Bonzini

Il 08/10/2014 10:29, Jan Kiszka ha scritto:

Quoted 22 lines

Hi all,

after migrating a Jailhouse VM to a newer host platform with shadow VMCS support I found a bug. As you may know, Jailhouse doesn't intercept interrupts, thus also never requests an interrupt window. Nevertheless:

 qemu-system-x86-5777  [001] 74970.625324: kvm_mmio:             mmio write len 4 gpa 0xfebf5008 val 0x20 qemu-system-x86-5777  [001] 74970.625325: kvm_userspace_exit:   reason KVM_EXIT_MMIO (6)
 qemu-system-x86-5777  [001] 74970.625330: kvm_entry:            vcpu 1
 qemu-system-x86-5777  [001] 74970.625333: kvm_exit:             reason PENDING_INTERRUPT rip 0xffffffff81043e54 info 0 0
 qemu-system-x86-5777  [001] 74970.625333: kvm_nested_vmexit:    rip: 0xffffffff81043e54 reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
 qemu-system-x86-5777  [001] 74970.625334: kvm_nested_vmexit_inject: reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
 qemu-system-x86-5777  [001] 74970.625339: kvm_entry:            vcpu 1
 qemu-system-x86-5777  [001] 74970.625341: kvm_exit:             reason EPT_MISCONFIG rip 0xfffffffff0002307 info 0 0
 qemu-system-x86-5777  [001] 74970.625343: kvm_emulate_insn:     0:fffffffff0002307:8b 40 20 (prot64)

And then Jailhouse crashes (which is also interesting to understand why
- L1 host state is corrupt). Anyway, the point is that we leak CPU_BASED_VIRTUAL_INTR_PENDING from L0 into vmcs12. L0 sets it before entering L2, and then we transfer it from the hardware state to vmc12 on exit because that VMCS field is shadowed. The crash disappears when disabling VMCS shadowing.

Quoted 6 lines
Quoted 2 lines

Can we simply stop shadowing CPU_BASED_VM_EXEC_CONTROL when CPU_BASED_VIRTUAL_INTR_PENDING is injected to L2?

The main problem is that we have a single shadowing bitmap for all virtual machines. I cannot think of a simple solution except not shadowing that field at all.

But that could be changed to per VCPU, no? For the time being, dropping the field from the shadowed list is likely the quickest fix.

Do we have more of such cases where the host writes to the hw state that is also used for shadowing? Guess it's good to have a second look...

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
#3 lore raw
In reply to Jan Kiszka

Hi Jan,

Quoted 15 lines · Jan Kiszka

Hi all,

after migrating a Jailhouse VM to a newer host platform with shadow VMCS support I found a bug. As you may know, Jailhouse doesn't intercept interrupts, thus also never requests an interrupt window. Nevertheless:

qemu-system-x86-5777  [001] 74970.625324: kvm_mmio:             mmio write len 4 gpa 0xfebf5008 val 0x20 qemu-system-x86-5777  [001] 74970.625325: kvm_userspace_exit:   reason KVM_EXIT_MMIO (6)
qemu-system-x86-5777  [001] 74970.625330: kvm_entry:            vcpu 1
qemu-system-x86-5777  [001] 74970.625333: kvm_exit:             reason PENDING_INTERRUPT rip 0xffffffff81043e54 info 0 0
qemu-system-x86-5777  [001] 74970.625333: kvm_nested_vmexit:    rip: 0xffffffff81043e54 reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
qemu-system-x86-5777  [001] 74970.625334: kvm_nested_vmexit_inject: reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
qemu-system-x86-5777  [001] 74970.625339: kvm_entry:            vcpu 1
qemu-system-x86-5777  [001] 74970.625341: kvm_exit:             reason EPT_MISCONFIG rip 0xfffffffff0002307 info 0 0
qemu-system-x86-5777  [001] 74970.625343: kvm_emulate_insn:     0:fffffffff0002307:8b 40 20 (prot64)
Quoted 3 lines

And then Jailhouse crashes (which is also interesting to understand why
- L1 host state is corrupt). Anyway, the point is that we leak CPU_BASED_VIRTUAL_INTR_PENDING from L0 into vmcs12. L0 sets it before

In prepare_vmcs02:

exec_control = vmx_exec_control(vmx); /* L0's desires */ exec_control &= ~CPU_BASED_VIRTUAL_INTR_PENDING; exec_control &= ~CPU_BASED_VIRTUAL_NMI_PENDING; exec_control &= ~CPU_BASED_TPR_SHADOW; exec_control |= vmcs12->cpu_based_vm_exec_control;

Could you point out where the other places L0 sets CPU_BASED_VIRTUAL_INTR_PENDING before entering L2?

Regards,
Wanpeng Li

Quoted 16 lines

entering L2, and then we transfer it from the hardware state to vmc12 on exit because that VMCS field is shadowed. The crash disappears when disabling VMCS shadowing.

Can we simply stop shadowing CPU_BASED_VM_EXEC_CONTROL when CPU_BASED_VIRTUAL_INTR_PENDING is injected to L2?

Jan

Signature · 6 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#4 lore raw
Quoted 23 lines · Wanpeng Li

Hi Jan,

Quoted 15 lines · Jan Kiszka

Hi all,

after migrating a Jailhouse VM to a newer host platform with shadow VMCS support I found a bug. As you may know, Jailhouse doesn't intercept interrupts, thus also never requests an interrupt window. Nevertheless:

qemu-system-x86-5777  [001] 74970.625324: kvm_mmio:             mmio write len 4 gpa 0xfebf5008 val 0x20 qemu-system-x86-5777  [001] 74970.625325: kvm_userspace_exit:   reason KVM_EXIT_MMIO (6)
qemu-system-x86-5777  [001] 74970.625330: kvm_entry:            vcpu 1
qemu-system-x86-5777  [001] 74970.625333: kvm_exit:             reason PENDING_INTERRUPT rip 0xffffffff81043e54 info 0 0
qemu-system-x86-5777  [001] 74970.625333: kvm_nested_vmexit:    rip: 0xffffffff81043e54 reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
qemu-system-x86-5777  [001] 74970.625334: kvm_nested_vmexit_inject: reason: PENDING_INTERRUPT ext_inf1: 0x0000000000000000 ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
qemu-system-x86-5777  [001] 74970.625339: kvm_entry:            vcpu 1
qemu-system-x86-5777  [001] 74970.625341: kvm_exit:             reason EPT_MISCONFIG rip 0xfffffffff0002307 info 0 0
qemu-system-x86-5777  [001] 74970.625343: kvm_emulate_insn:     0:fffffffff0002307:8b 40 20 (prot64)
Quoted 3 lines

And then Jailhouse crashes (which is also interesting to understand why
- L1 host state is corrupt). Anyway, the point is that we leak CPU_BASED_VIRTUAL_INTR_PENDING from L0 into vmcs12. L0 sets it before

In prepare_vmcs02:

Quoted 8 lines

exec_control = vmx_exec_control(vmx); /* L0's desires */ exec_control &= ~CPU_BASED_VIRTUAL_INTR_PENDING; exec_control &= ~CPU_BASED_VIRTUAL_NMI_PENDING; exec_control &= ~CPU_BASED_TPR_SHADOW; exec_control |= vmcs12->cpu_based_vm_exec_control;

Could you point out where the other places L0 sets CPU_BASED_VIRTUAL_INTR_PENDING before entering L2?

enable_irq_window(). I instrumented it, and it showed up right before vmcs12 state became corrupted.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
maintainer
#5 lore raw

Il 08/10/2014 11:51, Jan Kiszka ha scritto:

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

Could you point out where the other places L0 sets CPU_BASED_VIRTUAL_INTR_PENDING before entering L2?

enable_irq_window(). I instrumented it, and it showed up right before vmcs12 state became corrupted.

But it would write to the vmcs02, not to the shadow VMCS; the shadow VMCS is active during copy_shadow_to_vmcs12/copy_vmcs12_to_shadow, and at no other time. It is not clear to me how the VIRTUAL_INTR_PENDING bit ended up from the vmcs02 (where it is perfectly fine) to the vmcs12.

BTW, I think the two lines here that Wanpeng pointed out:

        exec_control = vmx_exec_control(vmx); /* L0's desires */
        exec_control &= ~CPU_BASED_VIRTUAL_INTR_PENDING;
        exec_control &= ~CPU_BASED_VIRTUAL_NMI_PENDING;

can be deleted, the bits will never be in vmx_exec_control(vmx), see setup_vmcs_config.

Paolo

author
#6 lore raw
Quoted 10 lines · Paolo Bonzini

Il 08/10/2014 11:51, Jan Kiszka ha scritto:

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

Could you point out where the other places L0 sets CPU_BASED_VIRTUAL_INTR_PENDING before entering L2?

enable_irq_window(). I instrumented it, and it showed up right before vmcs12 state became corrupted.

But it would write to the vmcs02, not to the shadow VMCS; the shadow VMCS is active during copy_shadow_to_vmcs12/copy_vmcs12_to_shadow, and at no other time. It is not clear to me how the VIRTUAL_INTR_PENDING bit ended up from the vmcs02 (where it is perfectly fine) to the vmcs12.

Well, but somehow that bit ends up in vmcs12, that's a fact. Also that the proble disappears when shadowing is disabled. Need to think about the path again. Maybe there is just a bug, not a conceptual issue.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
maintainer
#7 lore raw

Il 08/10/2014 12:29, Jan Kiszka ha scritto:

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

But it would write to the vmcs02, not to the shadow VMCS; the shadow VMCS is active during copy_shadow_to_vmcs12/copy_vmcs12_to_shadow, and at no other time. It is not clear to me how the VIRTUAL_INTR_PENDING bit ended up from the vmcs02 (where it is perfectly fine) to the vmcs12.

Well, but somehow that bit ends up in vmcs12, that's a fact. Also that the proble disappears when shadowing is disabled. Need to think about the path again. Maybe there is just a bug, not a conceptual issue.

Yeah, and at this point we cannot actually exclude a processor bug. Can you check that the bit is not in the shadow VMCS just before vmrun, or just after enable_irq_window?

Having a kvm-unit-tests testcase could also be of some help.

Paolo

author
#8 lore raw
Quoted 9 lines · Paolo Bonzini

Il 08/10/2014 12:29, Jan Kiszka ha scritto:

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

But it would write to the vmcs02, not to the shadow VMCS; the shadow VMCS is active during copy_shadow_to_vmcs12/copy_vmcs12_to_shadow, and at no other time. It is not clear to me how the VIRTUAL_INTR_PENDING bit ended up from the vmcs02 (where it is perfectly fine) to the vmcs12.

Well, but somehow that bit ends up in vmcs12, that's a fact. Also that the proble disappears when shadowing is disabled. Need to think about the path again. Maybe there is just a bug, not a conceptual issue.

Quoted 5 lines

Yeah, and at this point we cannot actually exclude a processor bug. Can you check that the bit is not in the shadow VMCS just before vmrun, or just after enable_irq_window?

Having a kvm-unit-tests testcase could also be of some help.

As usual, this was a nasty race that involved some concurrent VCPUs and proper host load, so hard to write unit tests...

arch/x86/kvm/vmx.c +8 -0
index 04fa1b8..d6bcaca 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

@@ -6457,6 +6461,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 	u64 field_value = 0;
 	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (q = 0; q < ARRAY_SIZE(fields); q++) {

@@ -6483,6 +6489,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 /*

No proper patch yet because there might be a smarter approach without using the preempt_disable() hammer. But the point is that we temporarily load a vmcs without updating loaded_vmcs->vmcs. Now, if some other VCPU is scheduling in right in the middle of this, the wrong vmcs will be flushed and then reloaded - e.g. a non-shadow vmcs with that interrupt window flag set...

Patch is currently under heavy load testing here, but it looks very good as the bug was quickly reproducible before I applied it.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
maintainer
#9 lore raw

Il 08/10/2014 17:07, Jan Kiszka ha scritto:

Quoted 42 lines

As usual, this was a nasty race that involved some concurrent VCPUs and proper host load, so hard to write unit tests...

arch/x86/kvm/vmx.c +8 -0
index 04fa1b8..d6bcaca 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

@@ -6457,6 +6461,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 	u64 field_value = 0;
 	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (q = 0; q < ARRAY_SIZE(fields); q++) {

@@ -6483,6 +6489,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
Quoted 2 lines

No proper patch yet because there might be a smarter approach without using the preempt_disable() hammer.

copy_vmcs12_to_shadow already runs with preemption disabled; for stable@ it's not that bad to do the same in copy_shadow_to_vmcs12.

For 3.18 it could be nice of course to use loaded_vmcs properly, but it would also incur some overhead.

Paolo

Quoted 8 lines

But the point is that we temporarily
load a vmcs without updating loaded_vmcs->vmcs. Now, if some other VCPU is scheduling in right in the middle of this, the wrong vmcs will be flushed and then reloaded - e.g. a non-shadow vmcs with that interrupt window flag set...

Patch is currently under heavy load testing here, but it looks very good as the bug was quickly reproducible before I applied it.

author
#10 lore raw
Quoted 43 lines · Paolo Bonzini

Il 08/10/2014 17:07, Jan Kiszka ha scritto:

Quoted 42 lines

As usual, this was a nasty race that involved some concurrent VCPUs and proper host load, so hard to write unit tests...

arch/x86/kvm/vmx.c +8 -0
index 04fa1b8..d6bcaca 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

@@ -6457,6 +6461,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 	u64 field_value = 0;
 	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (q = 0; q < ARRAY_SIZE(fields); q++) {

@@ -6483,6 +6489,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
Quoted 8 lines
Quoted 2 lines

No proper patch yet because there might be a smarter approach without using the preempt_disable() hammer.

copy_vmcs12_to_shadow already runs with preemption disabled; for stable@ it's not that bad to do the same in copy_shadow_to_vmcs12.

For 3.18 it could be nice of course to use loaded_vmcs properly, but it would also incur some overhead.

If the other direction is already under preempt_disable, I'm not sure if there is much to gain for this direction.

Anyway, fix sent.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
#11 lore raw
In reply to Jan Kiszka
Quoted 60 lines · Jan Kiszka
Quoted 9 lines · Paolo Bonzini

Il 08/10/2014 12:29, Jan Kiszka ha scritto:

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

But it would write to the vmcs02, not to the shadow VMCS; the shadow VMCS is active during copy_shadow_to_vmcs12/copy_vmcs12_to_shadow, and at no other time. It is not clear to me how the VIRTUAL_INTR_PENDING bit ended up from the vmcs02 (where it is perfectly fine) to the vmcs12.

Well, but somehow that bit ends up in vmcs12, that's a fact. Also that the proble disappears when shadowing is disabled. Need to think about the path again. Maybe there is just a bug, not a conceptual issue.

Quoted 5 lines

Yeah, and at this point we cannot actually exclude a processor bug. Can you check that the bit is not in the shadow VMCS just before vmrun, or just after enable_irq_window?

Having a kvm-unit-tests testcase could also be of some help.

As usual, this was a nasty race that involved some concurrent VCPUs and proper host load, so hard to write unit tests...

arch/x86/kvm/vmx.c +8 -0
index 04fa1b8..d6bcaca 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

@@ -6457,6 +6461,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 	u64 field_value = 0;
 	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (q = 0; q < ARRAY_SIZE(fields); q++) {

@@ -6483,6 +6489,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 /*
Quoted 6 lines

No proper patch yet because there might be a smarter approach without using the preempt_disable() hammer. But the point is that we temporarily load a vmcs without updating loaded_vmcs->vmcs. Now, if some other VCPU is scheduling in right in the middle of this, the wrong vmcs will be flushed and then reloaded - e.g. a non-shadow vmcs with that interrupt window flag set...

If non-shadow vmcs and shadow vmcs can present in one system simultaneously?

Regards,
Wanpeng Li

Quoted 13 lines

Patch is currently under heavy load testing here, but it looks very good as the bug was quickly reproducible before I applied it.

Jan

Signature · 6 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#12 lore raw
Quoted 61 lines · Wanpeng Li
Quoted 60 lines · Jan Kiszka
Quoted 9 lines · Paolo Bonzini

Il 08/10/2014 12:29, Jan Kiszka ha scritto:

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

But it would write to the vmcs02, not to the shadow VMCS; the shadow VMCS is active during copy_shadow_to_vmcs12/copy_vmcs12_to_shadow, and at no other time. It is not clear to me how the VIRTUAL_INTR_PENDING bit ended up from the vmcs02 (where it is perfectly fine) to the vmcs12.

Well, but somehow that bit ends up in vmcs12, that's a fact. Also that the proble disappears when shadowing is disabled. Need to think about the path again. Maybe there is just a bug, not a conceptual issue.

Quoted 5 lines

Yeah, and at this point we cannot actually exclude a processor bug. Can you check that the bit is not in the shadow VMCS just before vmrun, or just after enable_irq_window?

Having a kvm-unit-tests testcase could also be of some help.

As usual, this was a nasty race that involved some concurrent VCPUs and proper host load, so hard to write unit tests...

arch/x86/kvm/vmx.c +8 -0
index 04fa1b8..d6bcaca 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

@@ -6457,6 +6461,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 	u64 field_value = 0;
 	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (q = 0; q < ARRAY_SIZE(fields); q++) {

@@ -6483,6 +6489,8 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 /*
Quoted 8 lines
Quoted 6 lines

No proper patch yet because there might be a smarter approach without using the preempt_disable() hammer. But the point is that we temporarily load a vmcs without updating loaded_vmcs->vmcs. Now, if some other VCPU is scheduling in right in the middle of this, the wrong vmcs will be flushed and then reloaded - e.g. a non-shadow vmcs with that interrupt window flag set...

If non-shadow vmcs and shadow vmcs can present in one system simultaneously?

Ah, got it, you mean non-current-shadow vmcs.

Regards,
Wanpeng Li

Quoted 21 lines

Regards,
Wanpeng Li

Quoted 13 lines

Patch is currently under heavy load testing here, but it looks very good as the bug was quickly reproducible before I applied it.

Jan

Signature · 6 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html