[Qemu-devel] [PATCH] introduce on_vcpu

[Qemu-devel] [PATCH] introduce on_vcpu

[Glauber Costa]

on_vcpu is a qemu-kvm function that will make sure that a specific
piece of code will run on a requested cpu. We don't need that because
we're restricted to -smp 1 right now, but those days are likely to end soon.

So for the benefit of having qemu-kvm share more code with us, I'm
introducing our own version of on_vcpu(). Right now, we either run
a function on the current cpu, or abort the execution, because it would
mean something is seriously wrong.

As an example code, I "ported" kvm_update_guest_debug to use it,
with some slight differences from qemu-kvm.

This is probably 0.12 material

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |   37 +++++++++++++++++++++++++++++++------
 1 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 4e913e5..1d91f2e 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -39,6 +39,8 @@
     do { } while (0)
 #endif
 
+CPUState *current_env;
+
 typedef struct KVMSlot
 {
     target_phys_addr_t start_addr;
@@ -145,6 +147,14 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
 }
 
+static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
+{
+    if (env == current_env) {
+        func(data);
+        return;
+    }
+    assert(1);
+}
 
 int kvm_init_vcpu(CPUState *env)
 {
@@ -578,6 +588,7 @@ int kvm_cpu_exec(CPUState *env)
     int ret;
 
     dprintf("kvm_cpu_exec()\n");
+    current_env = env;
 
     do {
         if (env->exit_request) {
@@ -902,18 +913,32 @@ int kvm_sw_breakpoints_active(CPUState *env)
     return !TAILQ_EMPTY(&env->kvm_state->kvm_sw_breakpoints);
 }
 
+struct kvm_set_guest_debug_data {
+    struct kvm_guest_debug dbg;
+    CPUState *env;
+    int err;
+};
+
+static void kvm_invoke_set_guest_debug(void *data)
+{
+    struct kvm_set_guest_debug_data *dbg_data = data;
+    dbg_data->err = kvm_vcpu_ioctl(dbg_data->env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
+}
+
 int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
 {
-    struct kvm_guest_debug dbg;
+    struct kvm_set_guest_debug_data data;
 
-    dbg.control = 0;
+    data.dbg.control = 0;
     if (env->singlestep_enabled)
-        dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+        data.dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
 
-    kvm_arch_update_guest_debug(env, &dbg);
-    dbg.control |= reinject_trap;
+    kvm_arch_update_guest_debug(env, &data.dbg);
+    data.dbg.control |= reinject_trap;
+    data.env = env;
 
-    return kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg);
+    on_vcpu(env, kvm_invoke_set_guest_debug, &data);
+    return data.err;
 }
 
 int kvm_insert_breakpoint(CPUState *current_env, target_ulong addr,
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH] introduce on_vcpu

[Anthony Liguori]

Glauber Costa wrote:
> on_vcpu is a qemu-kvm function that will make sure that a specific
> piece of code will run on a requested cpu. We don't need that because
> we're restricted to -smp 1 right now, but those days are likely to end soon.
>
> So for the benefit of having qemu-kvm share more code with us, I'm
> introducing our own version of on_vcpu(). Right now, we either run
> a function on the current cpu, or abort the execution, because it would
> mean something is seriously wrong.
>
> As an example code, I "ported" kvm_update_guest_debug to use it,
> with some slight differences from qemu-kvm.
>
> This is probably 0.12 material
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  kvm-all.c |   37 +++++++++++++++++++++++++++++++------
>  1 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 4e913e5..1d91f2e 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -39,6 +39,8 @@
>      do { } while (0)
>  #endif
>  
> +CPUState *current_env;
> +
>  typedef struct KVMSlot
>  {
>      target_phys_addr_t start_addr;
> @@ -145,6 +147,14 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>  }
>  
> +static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> +{
> +    if (env == current_env) {
>   

Can't you just use cpu_single_env?

> +        func(data);
> +        return;
> +    }
> +    assert(1);
>   

Wouldn't assert(env == current_env) or abort() make more sense?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] introduce on_vcpu

[Anthony Liguori]

Glauber Costa wrote:
> Yes. Purpose of using curren_env was just to make it look more like qemu-kvm
> But I do understand that we'll have to change it anyway for this code to
> have any actual value, so I'm fine with using cpu_single_env.
>   

Yeah, if qemu-kvm is introduce another global current CPUState variable, 
then the qemu-kvm code has to change :-)

Since qemu-kvm keeps a thread id in CPUState, it would make more sense 
to check whether gettid() == env->thread_id, no?

>>> +        func(data);
>>> +        return;
>>> +    }
>>> +    assert(1);
>>>   
>>>       
>> Wouldn't assert(env == current_env) or abort() make more sense?
>>     
> abort() is fine. I can change it if you prefer.
>   

Please do.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] introduce on_vcpu

[Glauber Costa]

On Thu, Jul 16, 2009 at 04:19:54PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
>> on_vcpu is a qemu-kvm function that will make sure that a specific
>> piece of code will run on a requested cpu. We don't need that because
>> we're restricted to -smp 1 right now, but those days are likely to end soon.
>>
>> So for the benefit of having qemu-kvm share more code with us, I'm
>> introducing our own version of on_vcpu(). Right now, we either run
>> a function on the current cpu, or abort the execution, because it would
>> mean something is seriously wrong.
>>
>> As an example code, I "ported" kvm_update_guest_debug to use it,
>> with some slight differences from qemu-kvm.
>>
>> This is probably 0.12 material
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>> CC: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  kvm-all.c |   37 +++++++++++++++++++++++++++++++------
>>  1 files changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 4e913e5..1d91f2e 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -39,6 +39,8 @@
>>      do { } while (0)
>>  #endif
>>  +CPUState *current_env;
>> +
>>  typedef struct KVMSlot
>>  {
>>      target_phys_addr_t start_addr;
>> @@ -145,6 +147,14 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>>  }
>>  +static void on_vcpu(CPUState *env, void (*func)(void *data), void 
>> *data)
>> +{
>> +    if (env == current_env) {
>>   
>
> Can't you just use cpu_single_env?
Yes. Purpose of using curren_env was just to make it look more like qemu-kvm
But I do understand that we'll have to change it anyway for this code to
have any actual value, so I'm fine with using cpu_single_env.

>
>> +        func(data);
>> +        return;
>> +    }
>> +    assert(1);
>>   
>
> Wouldn't assert(env == current_env) or abort() make more sense?
abort() is fine. I can change it if you prefer.

Re: [Qemu-devel] [PATCH] introduce on_vcpu

[Anthony Liguori]

Glauber Costa wrote:
> On Thu, Jul 16, 2009 at 04:25:12PM -0500, Anthony Liguori wrote:
>   
>> Glauber Costa wrote:
>>     
>>> Yes. Purpose of using curren_env was just to make it look more like qemu-kvm
>>> But I do understand that we'll have to change it anyway for this code to
>>> have any actual value, so I'm fine with using cpu_single_env.
>>>   
>>>       
>> Yeah, if qemu-kvm is introduce another global current CPUState variable,  
>> then the qemu-kvm code has to change :-)
>>
>> Since qemu-kvm keeps a thread id in CPUState, it would make more sense  
>> to check whether gettid() == env->thread_id, no?
>>     
> disagree.
> gettid involves a syscall.

You're prematurely optimizing.  a gettid syscall likely costs around 
150-200 cycles.

>  current_env in qemu-kvm is a TLS variable.
> probably much cheaper.
>
>
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] introduce on_vcpu

[Glauber Costa]

On Thu, Jul 16, 2009 at 04:25:12PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
>> Yes. Purpose of using curren_env was just to make it look more like qemu-kvm
>> But I do understand that we'll have to change it anyway for this code to
>> have any actual value, so I'm fine with using cpu_single_env.
>>   
>
> Yeah, if qemu-kvm is introduce another global current CPUState variable,  
> then the qemu-kvm code has to change :-)
>
> Since qemu-kvm keeps a thread id in CPUState, it would make more sense  
> to check whether gettid() == env->thread_id, no?
disagree.
gettid involves a syscall. current_env in qemu-kvm is a TLS variable.
probably much cheaper.