Re: [Qemu-devel] [PATCH 04/19] cpu: call cpu_synchronize_post_init() from CPUClass.realize() if hotplugged

[Eduardo Habkost]

On Thu, Apr 11, 2013 at 04:51:43PM +0200, Igor Mammedov wrote:
> ... to synchronize CPU state to KVM
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> v2:
>   * linking kvm-stub.o to *-user target moved in separate patch
> ---
>  include/sysemu/kvm.h | 18 ++++++++++--------
>  kvm-all.c            |  1 +
>  kvm-stub.c           |  1 +
>  qom/cpu.c            |  4 ++++
>  vl.c                 |  1 -
>  5 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 93cef28..8fb2489 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -250,8 +250,6 @@ int kvm_check_extension(KVMState *s, unsigned int 
> extension);
>  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
>                                        uint32_t index, int reg);
>  void kvm_cpu_synchronize_state(CPUArchState *env);
> -void kvm_cpu_synchronize_post_reset(CPUState *cpu);
> -void kvm_cpu_synchronize_post_init(CPUState *cpu);
>  
>  /* generic hooks - to be moved/refactored once there are more users */
>  
> @@ -262,6 +260,16 @@ static inline void cpu_synchronize_state(CPUArchState 
> *env)
>      }
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
> +                                       hwaddr *phys_addr);
> +#endif
> +
> +#endif /* NEED_CPU_H */
> +
> +void kvm_cpu_synchronize_post_reset(CPUState *cpu);
> +void kvm_cpu_synchronize_post_init(CPUState *cpu);
> +
>  static inline void cpu_synchronize_post_reset(CPUState *cpu)
>  {
>      if (kvm_enabled()) {
> @@ -277,12 +285,6 @@ static inline void cpu_synchronize_post_init(CPUState 
> *cpu)
>  }
>  
>  
> -#if !defined(CONFIG_USER_ONLY)
> -int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
> -                                       hwaddr *phys_addr);
> -#endif
> -
> -#endif
>  int kvm_set_ioeventfd_mmio(int fd, uint32_t adr, uint32_t val, bool assign,
>                             uint32_t size);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index fc4e17c..1d17128 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -109,6 +109,7 @@ bool kvm_async_interrupts_allowed;
>  bool kvm_irqfds_allowed;
>  bool kvm_msi_via_irqfd_allowed;
>  bool kvm_gsi_routing_allowed;
> +bool kvm_allowed;
>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> diff --git a/kvm-stub.c b/kvm-stub.c
> index b34064a..8cb81c4 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -23,6 +23,7 @@ bool kvm_async_interrupts_allowed;
>  bool kvm_irqfds_allowed;
>  bool kvm_msi_via_irqfd_allowed;
>  bool kvm_gsi_routing_allowed;
> +bool kvm_allowed;
>  
>  int kvm_init_vcpu(CPUState *cpu)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index e242dcb..0c76712 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -20,6 +20,7 @@
>  
>  #include "qom/cpu.h"
>  #include "qemu-common.h"
> +#include "sysemu/kvm.h"
>  
>  void cpu_reset_interrupt(CPUState *cpu, int mask)
>  {
> @@ -57,6 +58,9 @@ static ObjectClass *cpu_common_class_by_name(const char 
> *cpu_model)
>  
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
> +    if (dev->hotplugged) {
> +        cpu_synchronize_post_init(CPU(dev));
> +    }

For reference: non-hotplugged CPUs have cpu_synchronize_post_init()
called from cpu_synchronize_all_post_init(), that is called from main(),
just after the machine->init() call.

That said, have you considered changing the code so that's possible to
call cpu_synchronize_post_init() for all CPUs, so we could kill the
cpu_synchronize_all_post_init() call entirely?

Maybe the solution for this is to realize non-hotplugged CPUs later,
instead of realizing them very early inside the machine init function.
If doing is not possible or not enough, we could use VM state notifiers
to watch for the right moment to call cpu_synchronize_post_init().

>  }
>  
>  static void cpu_class_init(ObjectClass *klass, void *data)
> diff --git a/vl.c b/vl.c
> index d694a90..21952c3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -267,7 +267,6 @@ static NotifierList machine_init_done_notifiers =
>      NOTIFIER_LIST_INITIALIZER(machine_init_done_notifiers);
>  
>  static bool tcg_allowed = true;
> -bool kvm_allowed;
>  bool xen_allowed;
>  uint32_t xen_domid;
>  enum xen_mode xen_mode = XEN_EMULATE;
> -- 
> 1.8.2
> 

-- 
Eduardo