[PATCH] Do not calculate linear rip in emulation failure report

[PATCH] Do not calculate linear rip in emulation failure report

[Glauber Costa]

If we're not gonna do anything (case in which failure is already
reported), we do not need to even bother with calculating the linear rip.

This is a nitpick, but I saw it while doing some testing, so here's
the patch.

Signed-off-by: Glauber Costa <gcosta@redhat.com>
---
 arch/x86/kvm/x86.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77fb2bd..191fef1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2026,11 +2026,11 @@ void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
 	unsigned long rip = vcpu->arch.rip;
 	unsigned long rip_linear;
 
-	rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
-
 	if (reported)
 		return;
 
+	rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
+
 	emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
 
 	printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
-- 
1.5.4.5

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Avi Kivity]

Glauber Costa wrote:
> If we're not gonna do anything (case in which failure is already
> reported), we do not need to even bother with calculating the linear rip.
>
> This is a nitpick, but I saw it while doing some testing, so here's
> the patch.
>
>   

Applied, thanks.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Mohammed Gamal]

On Tue, Jun 10, 2008 at 4:46 PM, Glauber Costa <gcosta@redhat.com> wrote:
> If we're not gonna do anything (case in which failure is already
> reported), we do not need to even bother with calculating the linear rip.
>
> This is a nitpick, but I saw it while doing some testing, so here's
> the patch.
>
> Signed-off-by: Glauber Costa <gcosta@redhat.com>
> ---
>  arch/x86/kvm/x86.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 77fb2bd..191fef1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2026,11 +2026,11 @@ void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
>        unsigned long rip = vcpu->arch.rip;
>        unsigned long rip_linear;
>
> -       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
> -
>        if (reported)
>                return;
>
> +       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
> +
>        emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
>
>        printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
> --
> 1.5.4.5

Why return immediately? Shouldn't we report on failure whenever it
occurs (i.e. by rather removing the if condition)?

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Glauber Costa]

On Fri, Jun 13, 2008 at 2:33 PM, Mohammed Gamal <m.gamal005@gmail.com> wrote:
> On Tue, Jun 10, 2008 at 4:46 PM, Glauber Costa <gcosta@redhat.com> wrote:
>> If we're not gonna do anything (case in which failure is already
>> reported), we do not need to even bother with calculating the linear rip.
>>
>> This is a nitpick, but I saw it while doing some testing, so here's
>> the patch.
>>
>> Signed-off-by: Glauber Costa <gcosta@redhat.com>
>> ---
>>  arch/x86/kvm/x86.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 77fb2bd..191fef1 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2026,11 +2026,11 @@ void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
>>        unsigned long rip = vcpu->arch.rip;
>>        unsigned long rip_linear;
>>
>> -       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
>> -
>>        if (reported)
>>                return;
>>
>> +       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
>> +
>>        emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
>>
>>        printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
>> --
>> 1.5.4.5
>
> Why return immediately? Shouldn't we report on failure whenever it
> occurs (i.e. by rather removing the if condition)?
>
the kernel ring buffer would be full very quickly, and this is not a
strong enough reason to do that ;-)
Most of the messages would be the same.

What I do think would be useful, and even planned to do (just have
more priority things waiting), is to move
the checking field to the vcpu struct. With the current setup, if you
kill your guest, set things up again, and run it,
you won't see any messages, even if it fails.

So the warning should really be once in the lifetime of the virtual
machine, not once in the lifetime of the kvm module.



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Avi Kivity]

Glauber Costa wrote:
>>>        unsigned long rip = vcpu->arch.rip;
>>>        unsigned long rip_linear;
>>>
>>> -       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
>>> -
>>>        if (reported)
>>>                return;
>>>
>>> +       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
>>> +
>>>        emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
>>>
>>>        printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
>>> --
>>> 1.5.4.5
>>>       
>> Why return immediately? Shouldn't we report on failure whenever it
>> occurs (i.e. by rather removing the if condition)?
>>
>>     
> the kernel ring buffer would be full very quickly, and this is not a
> strong enough reason to do that ;-)
> Most of the messages would be the same.
>
> What I do think would be useful, and even planned to do (just have
> more priority things waiting), is to move
> the checking field to the vcpu struct. With the current setup, if you
> kill your guest, set things up again, and run it,
> you won't see any messages, even if it fails.
>
> So the warning should really be once in the lifetime of the virtual
> machine, not once in the lifetime of the kvm module.
>
>
>   

I've changed it to use printk_ratelimit().

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Glauber Costa]

On Fri, Jun 13, 2008 at 4:46 PM, Avi Kivity <avi@qumranet.com> wrote:
> Glauber Costa wrote:
>>>>
>>>>       unsigned long rip = vcpu->arch.rip;
>>>>       unsigned long rip_linear;
>>>>
>>>> -       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
>>>> -
>>>>       if (reported)
>>>>               return;
>>>>
>>>> +       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
>>>> +
>>>>       emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
>>>>
>>>>       printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x
>>>> %02x\n",
>>>> --
>>>> 1.5.4.5
>>>>
>>>
>>> Why return immediately? Shouldn't we report on failure whenever it
>>> occurs (i.e. by rather removing the if condition)?
>>>
>>>
>>
>> the kernel ring buffer would be full very quickly, and this is not a
>> strong enough reason to do that ;-)
>> Most of the messages would be the same.
>>
>> What I do think would be useful, and even planned to do (just have
>> more priority things waiting), is to move
>> the checking field to the vcpu struct. With the current setup, if you
>> kill your guest, set things up again, and run it,
>> you won't see any messages, even if it fails.
>>
>> So the warning should really be once in the lifetime of the virtual
>> machine, not once in the lifetime of the kvm module.
>>
>>
>>
>
> I've changed it to use printk_ratelimit().

I've tested this option here before sending out the patch, since it
would address all issues.

But in error cases, it still seemed to generate too many messages.


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Avi Kivity]

Glauber Costa wrote:
>>
>> I've changed it to use printk_ratelimit().
>>     
>
> I've tested this option here before sending out the patch, since it
> would address all issues.
>
> But in error cases, it still seemed to generate too many messages.
>
>   

Isn't that a bug in printk_ratelimit(), then?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Glauber Costa]

On Fri, Jun 13, 2008 at 11:14 PM, Avi Kivity <avi@qumranet.com> wrote:
> Glauber Costa wrote:
>>>
>>> I've changed it to use printk_ratelimit().
>>>
>>
>> I've tested this option here before sending out the patch, since it
>> would address all issues.
>>
>> But in error cases, it still seemed to generate too many messages.
>>
>>
>
> Isn't that a bug in printk_ratelimit(), then?

I don't think so. It wasn't enough messages to DoS the system.
Just enough messages to annoy me.

That said, If the general feeling is that we should really be seeing
more than the first message, I'm okay with the ratelimit too.
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] Do not calculate linear rip in emulation failure report

[Avi Kivity]

Glauber Costa wrote:
> I don't think so. It wasn't enough messages to DoS the system.
> Just enough messages to annoy me.
>   

If emulation failure messages annoy you, I'm sure you can think of a way 
of removing them other than tampering with kvm_report_emulation_failure().



-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.