Patchwork [2/7] cpu model bug fixes and definition corrections: Allow an optional qemu_early_init_vcpu()

login
register
mail settings
Submitter john cooper
Date May 23, 2011, 9:46 p.m.
Message ID <4DDAD5C1.7060907@redhat.com>
Download mbox | patch
Permalink /patch/97096/
State New
Headers show

Comments

john cooper - May 23, 2011, 9:46 p.m.
Allow an optional qemu_early_init_vcpu() such that
kvm_arch_get_supported_cpuid() can be used from
cpu_x86_register().  Without this minimal setup
kvm_arch_get_supported_cpuid() gags kvm_ioctl() via
passing a NULL initialized KVMState *.

Signed-off-by: john cooper <john.cooper@redhat.com>
---
Jan Kiszka - May 24, 2011, 12:18 p.m.
Hi John,

a general remark first: KVM related patches usually flow via uq/master
of the qemu-kvm.git. Maintainers are Avi and Marcelo, so you need to
address them and include the kvm mailing list.

On 2011-05-23 23:46, john cooper wrote:
> Allow an optional qemu_early_init_vcpu() such that
> kvm_arch_get_supported_cpuid() can be used from
> cpu_x86_register().  Without this minimal setup
> kvm_arch_get_supported_cpuid() gags kvm_ioctl() via
> passing a NULL initialized KVMState *.
> 
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
> 
> diff --git a/cpus.c b/cpus.c
> index 1fc34b7..25122db 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -544,6 +544,14 @@ void qemu_main_loop_start(void)
>  {
>  }
>  
> +void qemu_early_init_vcpu(void *_env)
> +{
> +    CPUState *env = _env;
> +
> +    if (kvm_enabled())
> +        kvm_early_init_vcpu(env);

Please make sure that all your patches pass checkpatch.sh before posting.

> +}
> +
>  void qemu_init_vcpu(void *_env)
>  {
>      CPUState *env = _env;
> diff --git a/kvm-all.c b/kvm-all.c
> index 106eb3a..dc846aa 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -195,24 +195,46 @@ int kvm_pit_in_kernel(void)
>      return kvm_state->pit_in_kernel;
>  }
>  
> -int kvm_init_vcpu(CPUState *env)
> +/* env->kvm_state is needed early by kvm_check_extension()
> + * break it out so it may be setup early where needed
> + */
> +int kvm_early_init_vcpu(CPUState *env)
> +
>  {
>      KVMState *s = kvm_state;
> -    long mmap_size;
>      int ret;
>  
> -    DPRINTF("kvm_init_vcpu\n");
> +    DPRINTF("kvm_early_init_vcpu\n");
> +
> +    if (env->kvm_state) {      /* already setup */
> +        return 0;
> +    }
>  
>      ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
>      if (ret < 0) {
>          DPRINTF("kvm_create_vcpu failed\n");
> -        goto err;
> +    } else {
> +        env->kvm_fd = ret;
> +        env->kvm_state = s;
> +        env->kvm_vcpu_dirty = 1;
>      }
> +    return ret;
> +}
> +
> +int kvm_init_vcpu(CPUState *env)
> +{
> +    KVMState *s;
> +    long mmap_size;
> +    int ret;
> +
> +    DPRINTF("kvm_init_vcpu\n");
>  
> -    env->kvm_fd = ret;
> -    env->kvm_state = s;
> -    env->kvm_vcpu_dirty = 1;
> +    ret = kvm_early_init_vcpu(env);
> +    if (ret < 0) {
> +        goto err;
> +    }
>  
> +    s = env->kvm_state;
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>      if (mmap_size < 0) {
>          ret = mmap_size;
> diff --git a/kvm.h b/kvm.h
> index d565dba..fe0631b 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -53,6 +53,7 @@ int kvm_has_xcrs(void);
>  int kvm_has_many_ioeventfds(void);
>  
>  #ifdef NEED_CPU_H
> +int kvm_early_init_vcpu(CPUState *env);
>  int kvm_init_vcpu(CPUState *env);
>  
>  int kvm_cpu_exec(CPUState *env);
> diff --git a/qemu-common.h b/qemu-common.h
> index b851b20..2bea318 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -313,8 +313,10 @@ struct qemu_work_item {
>  };
>  
>  #ifdef CONFIG_USER_ONLY
> +#define qemu_early_init_vcpu(env) do { } while (0)
>  #define qemu_init_vcpu(env) do { } while (0)
>  #else
> +void qemu_early_init_vcpu(void *env);
>  void qemu_init_vcpu(void *env);
>  #endif
>  
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 89df997..73f44e8 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1261,6 +1261,7 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>              cpu_set_debug_excp_handler(breakpoint_handler);
>  #endif
>      }
> +    qemu_early_init_vcpu(env);

Lacking return code evaluation.

>      if (cpu_x86_register(env, cpu_model) < 0) {
>          cpu_x86_close(env);
>          return NULL;

However, this is the wrong approach IMHO. Neither
kvm_arch_get_supported_cpuid nor try_get_cpuid depend on CPUState or a
per-VCPU IOCTL. try_get_cpuid actually issues a KVM (ie. global) IOCTL.
So better pass KVMState around which is available even before VCPU creation.

Jan

Patch

diff --git a/cpus.c b/cpus.c
index 1fc34b7..25122db 100644
--- a/cpus.c
+++ b/cpus.c
@@ -544,6 +544,14 @@  void qemu_main_loop_start(void)
 {
 }
 
+void qemu_early_init_vcpu(void *_env)
+{
+    CPUState *env = _env;
+
+    if (kvm_enabled())
+        kvm_early_init_vcpu(env);
+}
+
 void qemu_init_vcpu(void *_env)
 {
     CPUState *env = _env;
diff --git a/kvm-all.c b/kvm-all.c
index 106eb3a..dc846aa 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -195,24 +195,46 @@  int kvm_pit_in_kernel(void)
     return kvm_state->pit_in_kernel;
 }
 
-int kvm_init_vcpu(CPUState *env)
+/* env->kvm_state is needed early by kvm_check_extension()
+ * break it out so it may be setup early where needed
+ */
+int kvm_early_init_vcpu(CPUState *env)
+
 {
     KVMState *s = kvm_state;
-    long mmap_size;
     int ret;
 
-    DPRINTF("kvm_init_vcpu\n");
+    DPRINTF("kvm_early_init_vcpu\n");
+
+    if (env->kvm_state) {      /* already setup */
+        return 0;
+    }
 
     ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
     if (ret < 0) {
         DPRINTF("kvm_create_vcpu failed\n");
-        goto err;
+    } else {
+        env->kvm_fd = ret;
+        env->kvm_state = s;
+        env->kvm_vcpu_dirty = 1;
     }
+    return ret;
+}
+
+int kvm_init_vcpu(CPUState *env)
+{
+    KVMState *s;
+    long mmap_size;
+    int ret;
+
+    DPRINTF("kvm_init_vcpu\n");
 
-    env->kvm_fd = ret;
-    env->kvm_state = s;
-    env->kvm_vcpu_dirty = 1;
+    ret = kvm_early_init_vcpu(env);
+    if (ret < 0) {
+        goto err;
+    }
 
+    s = env->kvm_state;
     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
     if (mmap_size < 0) {
         ret = mmap_size;
diff --git a/kvm.h b/kvm.h
index d565dba..fe0631b 100644
--- a/kvm.h
+++ b/kvm.h
@@ -53,6 +53,7 @@  int kvm_has_xcrs(void);
 int kvm_has_many_ioeventfds(void);
 
 #ifdef NEED_CPU_H
+int kvm_early_init_vcpu(CPUState *env);
 int kvm_init_vcpu(CPUState *env);
 
 int kvm_cpu_exec(CPUState *env);
diff --git a/qemu-common.h b/qemu-common.h
index b851b20..2bea318 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -313,8 +313,10 @@  struct qemu_work_item {
 };
 
 #ifdef CONFIG_USER_ONLY
+#define qemu_early_init_vcpu(env) do { } while (0)
 #define qemu_init_vcpu(env) do { } while (0)
 #else
+void qemu_early_init_vcpu(void *env);
 void qemu_init_vcpu(void *env);
 #endif
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 89df997..73f44e8 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1261,6 +1261,7 @@  CPUX86State *cpu_x86_init(const char *cpu_model)
             cpu_set_debug_excp_handler(breakpoint_handler);
 #endif
     }
+    qemu_early_init_vcpu(env);
     if (cpu_x86_register(env, cpu_model) < 0) {
         cpu_x86_close(env);
         return NULL;