Patchwork [11/15] kvm: Rework inner loop of kvm_cpu_exec

login
register
mail settings
Submitter Jan Kiszka
Date March 4, 2011, 10:20 a.m.
Message ID <481368ec2de108b87df4cd11c2bd870b215e49b5.1299233998.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/85406/
State New
Headers show

Comments

Jan Kiszka - March 4, 2011, 10:20 a.m.
Let kvm_cpu_exec return EXCP_* values consistently and generate those
codes already inside its inner loop. This means we will now re-enter the
kernel while ret == 0.

Update kvm_handle_internal_error accordingly, but keep
kvm_arch_handle_exit untouched, it will be converted in a separate step.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)
Marcelo Tosatti - March 5, 2011, 4:05 p.m.
On Fri, Mar 04, 2011 at 11:20:08AM +0100, Jan Kiszka wrote:
> Let kvm_cpu_exec return EXCP_* values consistently and generate those
> codes already inside its inner loop. This means we will now re-enter the
> kernel while ret == 0.
> 
> Update kvm_handle_internal_error accordingly, but keep
> kvm_arch_handle_exit untouched, it will be converted in a separate step.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  kvm-all.c |   26 ++++++++++++++------------
>  1 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 2952499..cc652cf 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -848,7 +848,7 @@ static int kvm_handle_internal_error(CPUState *env, struct kvm_run *run)
>          fprintf(stderr, "emulation failure\n");
>          if (!kvm_arch_stop_on_emulation_error(env)) {
>              cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> -            return 0;
> +            return EXCP_INTERRUPT;
>          }
>      }
>      /* FIXME: Should trigger a qmp message to let management know
> @@ -947,7 +947,7 @@ int kvm_cpu_exec(CPUState *env)
>  
>          if (ret == -EINTR || ret == -EAGAIN) {
>              DPRINTF("io window exit\n");
> -            ret = 0;
> +            ret = EXCP_INTERRUPT;
>              break;
>          }
>  
> @@ -956,7 +956,6 @@ int kvm_cpu_exec(CPUState *env)
>              abort();
>          }
>  
> -        ret = 0; /* exit loop */
>          switch (run->exit_reason) {

Better keep ret assignment here so default behaviour is to 
exit loop? EXCP_INTERRUPT.
Jan Kiszka - March 5, 2011, 6:12 p.m.
On 2011-03-05 17:05, Marcelo Tosatti wrote:
> On Fri, Mar 04, 2011 at 11:20:08AM +0100, Jan Kiszka wrote:
>> Let kvm_cpu_exec return EXCP_* values consistently and generate those
>> codes already inside its inner loop. This means we will now re-enter the
>> kernel while ret == 0.
>>
>> Update kvm_handle_internal_error accordingly, but keep
>> kvm_arch_handle_exit untouched, it will be converted in a separate step.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  kvm-all.c |   26 ++++++++++++++------------
>>  1 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 2952499..cc652cf 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -848,7 +848,7 @@ static int kvm_handle_internal_error(CPUState *env, struct kvm_run *run)
>>          fprintf(stderr, "emulation failure\n");
>>          if (!kvm_arch_stop_on_emulation_error(env)) {
>>              cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
>> -            return 0;
>> +            return EXCP_INTERRUPT;
>>          }
>>      }
>>      /* FIXME: Should trigger a qmp message to let management know
>> @@ -947,7 +947,7 @@ int kvm_cpu_exec(CPUState *env)
>>  
>>          if (ret == -EINTR || ret == -EAGAIN) {
>>              DPRINTF("io window exit\n");
>> -            ret = 0;
>> +            ret = EXCP_INTERRUPT;
>>              break;
>>          }
>>  
>> @@ -956,7 +956,6 @@ int kvm_cpu_exec(CPUState *env)
>>              abort();
>>          }
>>  
>> -        ret = 0; /* exit loop */
>>          switch (run->exit_reason) {
> 
> Better keep ret assignment here so default behaviour is to 
> exit loop? EXCP_INTERRUPT.

There is no real default behavior: in two cases we stay in the loop, in
two others we leave, and the rest obtains ret from a return value.
Moreover, if a new case misses to set ret, the compiler will complain.

Jan
Marcelo Tosatti - March 11, 2011, 9:34 p.m.
On Sat, Mar 05, 2011 at 07:12:50PM +0100, Jan Kiszka wrote:
> >> @@ -956,7 +956,6 @@ int kvm_cpu_exec(CPUState *env)
> >>              abort();
> >>          }
> >>  
> >> -        ret = 0; /* exit loop */
> >>          switch (run->exit_reason) {
> > 
> > Better keep ret assignment here so default behaviour is to 
> > exit loop? EXCP_INTERRUPT.
> 
> There is no real default behavior: in two cases we stay in the loop, in
> two others we leave, and the rest obtains ret from a return value.
> Moreover, if a new case misses to set ret, the compiler will complain.
> 
> Jan

It will not complain because "ret" is used to store return value
of KVM_RUN.
Jan Kiszka - March 12, 2011, 9:16 a.m.
On 2011-03-11 22:34, Marcelo Tosatti wrote:
> On Sat, Mar 05, 2011 at 07:12:50PM +0100, Jan Kiszka wrote:
>>>> @@ -956,7 +956,6 @@ int kvm_cpu_exec(CPUState *env)
>>>>              abort();
>>>>          }
>>>>  
>>>> -        ret = 0; /* exit loop */
>>>>          switch (run->exit_reason) {
>>>
>>> Better keep ret assignment here so default behaviour is to 
>>> exit loop? EXCP_INTERRUPT.
>>
>> There is no real default behavior: in two cases we stay in the loop, in
>> two others we leave, and the rest obtains ret from a return value.
>> Moreover, if a new case misses to set ret, the compiler will complain.
>>
>> Jan
> 
> It will not complain because "ret" is used to store return value
> of KVM_RUN.
> 

Right, I'll disentangle this duplicate use of 'ret' (writing patches is
likely much better than watching more news this morning...).

Jan

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 2952499..cc652cf 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -848,7 +848,7 @@  static int kvm_handle_internal_error(CPUState *env, struct kvm_run *run)
         fprintf(stderr, "emulation failure\n");
         if (!kvm_arch_stop_on_emulation_error(env)) {
             cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
-            return 0;
+            return EXCP_INTERRUPT;
         }
     }
     /* FIXME: Should trigger a qmp message to let management know
@@ -947,7 +947,7 @@  int kvm_cpu_exec(CPUState *env)
 
         if (ret == -EINTR || ret == -EAGAIN) {
             DPRINTF("io window exit\n");
-            ret = 0;
+            ret = EXCP_INTERRUPT;
             break;
         }
 
@@ -956,7 +956,6 @@  int kvm_cpu_exec(CPUState *env)
             abort();
         }
 
-        ret = 0; /* exit loop */
         switch (run->exit_reason) {
         case KVM_EXIT_IO:
             DPRINTF("handle_io\n");
@@ -965,7 +964,7 @@  int kvm_cpu_exec(CPUState *env)
                           run->io.direction,
                           run->io.size,
                           run->io.count);
-            ret = 1;
+            ret = 0;
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");
@@ -973,14 +972,16 @@  int kvm_cpu_exec(CPUState *env)
                                    run->mmio.data,
                                    run->mmio.len,
                                    run->mmio.is_write);
-            ret = 1;
+            ret = 0;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
             DPRINTF("irq_window_open\n");
+            ret = EXCP_INTERRUPT;
             break;
         case KVM_EXIT_SHUTDOWN:
             DPRINTF("shutdown\n");
             qemu_system_reset_request();
+            ret = EXCP_INTERRUPT;
             break;
         case KVM_EXIT_UNKNOWN:
             fprintf(stderr, "KVM: unknown exit, hardware reason %" PRIx64 "\n",
@@ -997,28 +998,29 @@  int kvm_cpu_exec(CPUState *env)
             DPRINTF("kvm_exit_debug\n");
             if (kvm_arch_debug(&run->debug.arch)) {
                 ret = EXCP_DEBUG;
-                goto out;
+                break;
             }
             /* re-enter, this exception was guest-internal */
-            ret = 1;
+            ret = 0;
             break;
 #endif /* KVM_CAP_SET_GUEST_DEBUG */
         default:
             DPRINTF("kvm_arch_handle_exit\n");
             ret = kvm_arch_handle_exit(env, run);
+            if (ret == 0) {
+                ret = EXCP_INTERRUPT;
+            } else if (ret > 0) {
+                ret = 0;
+            }
             break;
         }
-    } while (ret > 0);
+    } while (ret == 0);
 
     if (ret < 0) {
         cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
         vm_stop(VMSTOP_PANIC);
     }
-    ret = EXCP_INTERRUPT;
 
-#ifdef KVM_CAP_SET_GUEST_DEBUG
-out:
-#endif
     env->exit_request = 0;
     cpu_single_env = NULL;
     return ret;