Re: [Qemu-devel] [PATCH 2/7] cpu model bug fixes and definition corrections: Allow an optional qemu_early_init_vcpu()

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux