Patchwork [v2,06/20] kvm: Install optimized interrupt handler

login
register
mail settings
Submitter Jan Kiszka
Date March 15, 2011, 11:26 a.m.
Message ID <b8c5dfe9505a48556ee17545a53e8f6245e4b262.1300188374.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/86950/
State New
Headers show

Comments

Jan Kiszka - March 15, 2011, 11:26 a.m.
KVM only requires to set the raised IRQ in CPUState and to kick the
receiving vcpu if it is remote.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
Marcelo Tosatti - March 15, 2011, 5:10 p.m.
On Tue, Mar 15, 2011 at 12:26:17PM +0100, Jan Kiszka wrote:
> KVM only requires to set the raised IRQ in CPUState and to kick the
> receiving vcpu if it is remote.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  kvm-all.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 226843c..25ab545 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -650,6 +650,15 @@ static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
>      .log_stop = kvm_log_stop,
>  };
>  
> +static void kvm_handle_interrupt(CPUState *env, int mask)
> +{
> +    env->interrupt_request |= mask;
> +
> +    if (!qemu_cpu_is_self(env)) {
> +        qemu_cpu_kick(env);
> +    }
> +}
> +

Not sure its worthwhile to allow different handlers. The advantage over
tcg version is that its shorter, without cpu_unlink_tb and icount
handler?
Jan Kiszka - March 15, 2011, 8:12 p.m.
On 2011-03-15 18:10, Marcelo Tosatti wrote:
> On Tue, Mar 15, 2011 at 12:26:17PM +0100, Jan Kiszka wrote:
>> KVM only requires to set the raised IRQ in CPUState and to kick the
>> receiving vcpu if it is remote.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  kvm-all.c |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 226843c..25ab545 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -650,6 +650,15 @@ static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
>>      .log_stop = kvm_log_stop,
>>  };
>>  
>> +static void kvm_handle_interrupt(CPUState *env, int mask)
>> +{
>> +    env->interrupt_request |= mask;
>> +
>> +    if (!qemu_cpu_is_self(env)) {
>> +        qemu_cpu_kick(env);
>> +    }
>> +}
>> +
> 
> Not sure its worthwhile to allow different handlers. The advantage over
> tcg version is that its shorter, without cpu_unlink_tb and icount
> handler?

I thought about this again as well before posting, and I came to the
conclusion that an important advantage is avoiding TCG surprises in KVM
code paths. This way, KVM does not need to bother if cpu_unlink_tb or
icount related code changes. Maybe I should have added this to the
commit message.

Jan
Jan Kiszka - March 18, 2011, 10:18 a.m.
On 2011-03-15 21:12, Jan Kiszka wrote:
> On 2011-03-15 18:10, Marcelo Tosatti wrote:
>> On Tue, Mar 15, 2011 at 12:26:17PM +0100, Jan Kiszka wrote:
>>> KVM only requires to set the raised IRQ in CPUState and to kick the
>>> receiving vcpu if it is remote.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  kvm-all.c |   11 +++++++++++
>>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 226843c..25ab545 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -650,6 +650,15 @@ static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
>>>      .log_stop = kvm_log_stop,
>>>  };
>>>  
>>> +static void kvm_handle_interrupt(CPUState *env, int mask)
>>> +{
>>> +    env->interrupt_request |= mask;
>>> +
>>> +    if (!qemu_cpu_is_self(env)) {
>>> +        qemu_cpu_kick(env);
>>> +    }
>>> +}
>>> +
>>
>> Not sure its worthwhile to allow different handlers. The advantage over
>> tcg version is that its shorter, without cpu_unlink_tb and icount
>> handler?
> 
> I thought about this again as well before posting, and I came to the
> conclusion that an important advantage is avoiding TCG surprises in KVM
> code paths. This way, KVM does not need to bother if cpu_unlink_tb or
> icount related code changes. Maybe I should have added this to the
> commit message.

What's your opinion on this? Should I repost the remaining three with
comments adjusted?

Jan
Marcelo Tosatti - March 18, 2011, 11:29 a.m.
On Fri, Mar 18, 2011 at 11:18:40AM +0100, Jan Kiszka wrote:
> On 2011-03-15 21:12, Jan Kiszka wrote:
> > On 2011-03-15 18:10, Marcelo Tosatti wrote:
> >> On Tue, Mar 15, 2011 at 12:26:17PM +0100, Jan Kiszka wrote:
> >>> KVM only requires to set the raised IRQ in CPUState and to kick the
> >>> receiving vcpu if it is remote.
> >>>
> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>> ---
> >>>  kvm-all.c |   11 +++++++++++
> >>>  1 files changed, 11 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index 226843c..25ab545 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -650,6 +650,15 @@ static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
> >>>      .log_stop = kvm_log_stop,
> >>>  };
> >>>  
> >>> +static void kvm_handle_interrupt(CPUState *env, int mask)
> >>> +{
> >>> +    env->interrupt_request |= mask;
> >>> +
> >>> +    if (!qemu_cpu_is_self(env)) {
> >>> +        qemu_cpu_kick(env);
> >>> +    }
> >>> +}
> >>> +
> >>
> >> Not sure its worthwhile to allow different handlers. The advantage over
> >> tcg version is that its shorter, without cpu_unlink_tb and icount
> >> handler?
> > 
> > I thought about this again as well before posting, and I came to the
> > conclusion that an important advantage is avoiding TCG surprises in KVM
> > code paths. This way, KVM does not need to bother if cpu_unlink_tb or
> > icount related code changes. Maybe I should have added this to the
> > commit message.
> 
> What's your opinion on this? Should I repost the remaining three with
> comments adjusted?
> 
> Jan
>

Its up to you. Your argument above makes sense.

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 226843c..25ab545 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -650,6 +650,15 @@  static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
     .log_stop = kvm_log_stop,
 };
 
+static void kvm_handle_interrupt(CPUState *env, int mask)
+{
+    env->interrupt_request |= mask;
+
+    if (!qemu_cpu_is_self(env)) {
+        qemu_cpu_kick(env);
+    }
+}
+
 int kvm_init(void)
 {
     static const char upgrade_note[] =
@@ -758,6 +767,8 @@  int kvm_init(void)
 
     s->many_ioeventfds = kvm_check_many_ioeventfds();
 
+    cpu_interrupt_handler = kvm_handle_interrupt;
+
     return 0;
 
 err: