KVM: Add missing emulation failure report in kvm_mmu_page_fault()

PATCH In review Sheng Yang 3 participants 8 messages Jan 15, 2010 – Jan 19, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Sheng Yang

author

2010-01-15 08:44 UTC #0 lore raw

PATCH 1/1 KVM: Add missing emulation failure report in kvm_mmu_page_fault()

Currently we only have handle_invalid_guest_state() reported emulation failure...

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/mmu.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index 4f5508c..037e52a 100644
@@ -2791,6 +2791,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code)
 		++vcpu->stat.mmio_exits;
 		return 0;
 	case EMULATE_FAIL:
+		kvm_report_emulation_failure(vcpu, "emulation failure");
 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
 		vcpu->run->internal.ndata = 0;

Signature · 1 line

1.5.4.5

Avi Kivity

2010-01-17 12:34 UTC #1 lore raw

Quoted 13 lines · Sheng Yang

Currently we only have handle_invalid_guest_state() reported emulation failure...
Signed-off-by Sheng Yang<[email protected]>
---
  arch/x86/kvm/mmu.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
arch/x86/kvm/mmu.c +0 -0
index 4f5508c..037e52a 100644

arch/x86/kvm/mmu.c int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code) · line 2797

Quoted 7 lines

arch/x86/kvm/mmu.c +1 -0

@@ -2791,6 +2791,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code)
  		++vcpu->stat.mmio_exits;
  		return 0;
  	case EMULATE_FAIL:
+		kvm_report_emulation_failure(vcpu, "emulation failure");
  		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
  		vcpu->run->internal.ndata = 0;

This is intentional - instead of spamming dmesg, we exit with an internal error. Modern qemu-kvm will halt and allow the user to inspect the guest with the built-in disassembler.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2010-01-18 09:32 UTC #2 lore raw

Quoted 9 lines · On Sunday 17 January 2010 20:34:23 Avi Kivity wrote:

Quoted 8 lines · Sheng Yang
Currently we only have handle_invalid_guest_state() reported emulation failure...
Signed-off-by Sheng Yang<[email protected]>
---
  arch/x86/kvm/mmu.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

Quoted 17 lines

arch/x86/kvm/mmu.c int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t · line 2797
Quoted 12 lines
arch/x86/kvm/mmu.c +1 -0
index 4f5508c..037e52a 100644
@@ -2791,6 +2791,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t
 cr2, u32 error_code) ++vcpu->stat.mmio_exits;
  		return 0;
  	case EMULATE_FAIL:
+		kvm_report_emulation_failure(vcpu, "emulation failure");
  		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
  		vcpu->run->internal.ndata = 0;
This is intentional - instead of spamming dmesg, we exit with an internal error. Modern qemu-kvm will halt and allow the user to inspect the guest with the built-in disassembler.

I think keep it there still useful for some users. And we have the same report in handle_invalid_guest_state(), and we even have "emulation failure, check dmesg for details" in QEmu when handling KVM_INTERNAL_ERROR_EMULATION.

I think add one line here is the easiest way to keep consistence, and is handy.

Signature · 2 lines

regards
Yang, Sheng

Avi Kivity

2010-01-18 11:32 UTC #3 lore raw

Quoted 12 lines · Sheng Yang

Quoted 10 lines · On Sunday 17 January 2010 20:34:23 Avi Kivity wrote:
Quoted 3 lines · Sheng Yang
Currently we only have handle_invalid_guest_state() reported emulation failure...
This is intentional - instead of spamming dmesg, we exit with an internal error. Modern qemu-kvm will halt and allow the user to inspect the guest with the built-in disassembler.

Quoted 7 lines

I think keep it there still useful for some users. And we have the same report in handle_invalid_guest_state(), and we even have "emulation failure, check dmesg for details" in QEmu when handling KVM_INTERNAL_ERROR_EMULATION.
I think add one line here is the easiest way to keep consistence, and is handy.

Another way to keep consistency is to remove emulation failure reporting in handle_invalid_guest_state() :)

There are two problems with the kernel failure report. First, it doesn't report enough data - registers, surrounding instructions, etc. that are needed to explain what is going on. Second, it can flood dmesg, which is a pretty bad thing to do.

I have a patch somewhere that adds instruction emulation bytes (both successful and failed) to ftrace. That may be useful, perhaps.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2010-01-19 03:06 UTC #4 lore raw

Quoted 15 lines · On Monday 18 January 2010 19:32:14 Avi Kivity wrote:

Quoted 14 lines · Sheng Yang
Quoted 7 lines · On Sunday 17 January 2010 20:34:23 Avi Kivity wrote:
Quoted 2 lines · Sheng Yang
Currently we only have handle_invalid_guest_state() reported emulation failure...
This is intentional - instead of spamming dmesg, we exit with an internal error. Modern qemu-kvm will halt and allow the user to inspect the guest with the built-in disassembler.
I think keep it there still useful for some users. And we have the same report in handle_invalid_guest_state(), and we even have "emulation failure, check dmesg for details" in QEmu when handling KVM_INTERNAL_ERROR_EMULATION.

Quoted 5 lines

Quoted 2 lines
I think add one line here is the easiest way to keep consistence, and is handy.
Another way to keep consistency is to remove emulation failure reporting in handle_invalid_guest_state() :)

OK, I would remove it...

Quoted 4 lines

There are two problems with the kernel failure report. First, it doesn't report enough data - registers, surrounding instructions, etc. that are needed to explain what is going on. Second, it can flood dmesg, which is a pretty bad thing to do.

When you talking about "built-in disassembler", do you talking about "memsave/objdump" or some other more convenient way for this?

And maybe we can let QEmu do some dump of the assembler code? (kvm_show_code())

Quoted 2 lines

I have a patch somewhere that adds instruction emulation bytes (both successful and failed) to ftrace. That may be useful, perhaps.

It would surely help. :)

Signature · 2 lines

regards
Yang, Sheng

Avi Kivity

2010-01-19 07:54 UTC #5 lore raw

Quoted 7 lines · Sheng Yang

Quoted 6 lines
There are two problems with the kernel failure report. First, it doesn't report enough data - registers, surrounding instructions, etc. that are needed to explain what is going on. Second, it can flood dmesg, which is a pretty bad thing to do.

Quoted 6 lines

When you talking about "built-in disassembler", do you talking about "memsave/objdump" or some other more convenient way for this?
And maybe we can let QEmu do some dump of the assembler code? (kvm_show_code())

In the qemu monitor, type 'x/20i $eip' to get a disassembly of code around the current instruction pointer.

Quoted 5 lines

Quoted 3 lines
I have a patch somewhere that adds instruction emulation bytes (both successful and failed) to ftrace. That may be useful, perhaps.
It would surely help. :)

I'll try to resurrect it.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2010-01-19 07:58 UTC #6 lore raw

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Sheng Yang
Quoted 5 lines
There are two problems with the kernel failure report. First, it doesn't report enough data - registers, surrounding instructions, etc. that are needed to explain what is going on. Second, it can flood dmesg, which is a pretty bad thing to do.

Quoted 9 lines

Quoted 5 lines
When you talking about "built-in disassembler", do you talking about "memsave/objdump" or some other more convenient way for this?
And maybe we can let QEmu do some dump of the assembler code? (kvm_show_code())
In the qemu monitor, type 'x/20i $eip' to get a disassembly of code around the current instruction pointer.

If you want to see code _around_ the current instruction pointer type 'x/20i $eip-20' :) Or that is what I usually do.

Quoted 13 lines

Quoted 3 lines
Quoted 2 lines
I have a patch somewhere that adds instruction emulation bytes (both successful and failed) to ftrace. That may be useful, perhaps.
It would surely help. :)
I'll try to resurrect it.
Signature · 6 lines
error compiling committee.c: too many arguments to function

--
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

--
Gleb.

Sheng Yang

author

2010-01-19 08:05 UTC #7 lore raw

Quoted 10 lines · On Tuesday 19 January 2010 15:57:57 Gleb Natapov wrote:

Quoted 9 lines · Avi Kivity
Quoted 8 lines · Sheng Yang
Quoted 4 lines
There are two problems with the kernel failure report. First, it doesn't report enough data - registers, surrounding instructions, etc. that are needed to explain what is going on. Second, it can flood dmesg, which is a pretty bad thing to do.
When you talking about "built-in disassembler", do you talking about "memsave/objdump" or some other more convenient way for this?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
And maybe we can let QEmu do some dump of the assembler code? (kvm_show_code())
In the qemu monitor, type 'x/20i $eip' to get a disassembly of code around the current instruction pointer.
If you want to see code _around_ the current instruction pointer type 'x/20i $eip-20' :) Or that is what I usually do.

Oh, damn, indeed very convenient...

Thanks you all for this good tip. :)

Signature · 2 lines

regards
Yang, Sheng