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

[Eduardo Habkost]

On Fri, Apr 12, 2013 at 01:34:09PM +0200, Igor Mammedov wrote:
> On Thu, 11 Apr 2013 15:33:01 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > 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().
> 
> I have not considered it, since question is out of scope CPU hot-plug.
> It might be feasible to do so later in separate patch and remove
> "if (dev->hotplugged)" condition tough.
> 
> I think it will be more appropriate when machine will QOM-ified, then
> to make machine.realize() work it had to be done, but now I don't see
> any gain doing so (providing all the trouble it might cause).

OK, sounds like a good justification to me.

I would love to have a device model where there's no difference between
hotplugged and non-hotplugged devices. But this is hard to achieve.

-- 
Eduardo