[PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Sheng Yang]

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

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/mmu.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 4f5508c..037e52a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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;
-- 
1.5.4.5

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Avi Kivity]

On 01/15/2010 10:44 AM, Sheng Yang wrote:
> Currently we only have handle_invalid_guest_state() reported emulation failure...
>
> Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> ---
>   arch/x86/kvm/mmu.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 4f5508c..037e52a 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -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.

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

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Sheng Yang]

On Sunday 17 January 2010 20:34:23 Avi Kivity wrote:
> On 01/15/2010 10:44 AM, Sheng Yang wrote:
> > Currently we only have handle_invalid_guest_state() reported emulation
> > failure...
> >
> > Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> > ---
> >   arch/x86/kvm/mmu.c |    1 +
> >   1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 4f5508c..037e52a 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -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.

-- 
regards
Yang, Sheng

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Avi Kivity]

On 01/18/2010 11:32 AM, Sheng Yang wrote:
> On Sunday 17 January 2010 20:34:23 Avi Kivity wrote:
>    
>> On 01/15/2010 10:44 AM, Sheng Yang wrote:
>>      
>>> 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.
>
> 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.

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

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Sheng Yang]

On Monday 18 January 2010 19:32:14 Avi Kivity wrote:
> On 01/18/2010 11:32 AM, Sheng Yang wrote:
> > On Sunday 17 January 2010 20:34:23 Avi Kivity wrote:
> >> On 01/15/2010 10:44 AM, Sheng Yang wrote:
> >>> 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.
> >
> > 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...
 
> 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())

> 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. :)

-- 
regards
Yang, Sheng

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Avi Kivity]

On 01/19/2010 05:06 AM, Sheng Yang wrote:
>
>> 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())
>    

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

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

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

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Gleb Natapov]

On Tue, Jan 19, 2010 at 09:54:44AM +0200, Avi Kivity wrote:
> On 01/19/2010 05:06 AM, Sheng Yang wrote:
> >
> >>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())
> 
> 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.

> >>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.
> 
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH] KVM: Add missing emulation failure report in kvm_mmu_page_fault()

[Sheng Yang]

On Tuesday 19 January 2010 15:57:57 Gleb Natapov wrote:
> On Tue, Jan 19, 2010 at 09:54:44AM +0200, Avi Kivity wrote:
> > On 01/19/2010 05:06 AM, Sheng Yang wrote:
> > >>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())
> >
> > 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. :)

-- 
regards
Yang, Sheng