Re: [Qemu-devel] [PATCH 2/2] LAPIC: make lapic support cpu hotplug

Re: [Qemu-devel] [PATCH 2/2] LAPIC: make lapic support cpu hotplug

[Anthony Liguori]

On 10/04/2011 06:13 AM, pingfank@linux.vnet.com wrote:
> From: Liu Ping Fan<pingfank@linux.vnet.ibm.com>
>
> Separate apic from qbus to icc bus which supports hotplug feature.
> And make the creation of apic as part of cpu initialization, so
> apic's state has been ready, before setting kvm_apic.
>
> Signed-off-by: Liu Ping Fan<pingfank@linux.vnet.ibm.com>
> ---
>   Makefile.target      |    1 +
>   hw/apic.c            |    7 ++++-
>   hw/apic.h            |    1 +
>   hw/icc_bus.c         |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/icc_bus.h         |   24 +++++++++++++++++++
>   hw/pc.c              |   11 ++++-----
>   target-i386/cpu.h    |    1 +
>   target-i386/helper.c |    7 +++++-
>   target-i386/kvm.c    |    1 -
>   9 files changed, 105 insertions(+), 10 deletions(-)
>   create mode 100644 hw/icc_bus.c
>   create mode 100644 hw/icc_bus.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 9011f28..5607c6d 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>   obj-i386-y += testdev.o
>   obj-i386-y += acpi.o acpi_piix4.o
> +obj-i386-y += icc_bus.o
>
>   obj-i386-y += pcspk.o i8254.o
>   obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> diff --git a/hw/apic.c b/hw/apic.c
> index 69d6ac5..95a1664 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -24,6 +24,7 @@
>   #include "sysbus.h"
>   #include "trace.h"
>   #include "kvm.h"
> +#include "icc_bus.h"
>
>   /* APIC Local Vector Table */
>   #define APIC_LVT_TIMER   0
> @@ -304,11 +305,13 @@ void cpu_set_apic_base(DeviceState *d, uint64_t val)
>
>       if (!s)
>           return;
> +
>       if (kvm_enabled()&&  kvm_irqchip_in_kernel())
>           s->apicbase = val;
>       else
>           s->apicbase = (val&  0xfffff000) |
>               (s->apicbase&  (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE));
> +
>       /* if disabled, cannot be enabled again */
>       if (!(val&  MSR_IA32_APICBASE_ENABLE)) {
>           s->apicbase&= ~MSR_IA32_APICBASE_ENABLE;
> @@ -1075,7 +1078,7 @@ static const VMStateDescription vmstate_apic = {
>       }
>   };

Please don't introduce extra whitespace in unrelated code.

>
> -static void apic_reset(DeviceState *d)
> +void apic_reset(DeviceState *d)
>   {
>       APICState *s = DO_UPCAST(APICState, busdev.qdev, d);
>       int bsp;
> @@ -1138,7 +1141,7 @@ static SysBusDeviceInfo apic_info = {
>
>   static void apic_register_devices(void)
>   {
> -    sysbus_register_withprop(&apic_info);
> +    iccbus_register(&apic_info);
>   }
>
>   device_init(apic_register_devices)
> diff --git a/hw/apic.h b/hw/apic.h
> index c857d52..e258efa 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -24,5 +24,6 @@ void apic_sipi(DeviceState *s);
>   /* pc.c */
>   int cpu_is_bsp(CPUState *env);
>   DeviceState *cpu_get_current_apic(void);
> +void apic_reset(DeviceState *d);
>
>   #endif
> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> new file mode 100644
> index 0000000..360ca2a
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,62 @@
> +/*
> +*/

New files need to include copyright/licenses.

> +#define ICC_BUS_PLUG
> +#ifdef ICC_BUS_PLUG

Drop these guards here and at the end of the file.  We conditionally build files 
by having an:

obj-$(CONFIG_ICC_BUS) += icc_bus.o

In the Makefile.

> +#include "icc_bus.h"

> +
> +
> +
> +struct icc_bus_info icc_info = {
> +    .qinfo.name = "icc",
> +    .qinfo.size = sizeof(struct icc_bus),
> +    .qinfo.props = (Property[]) {
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +
> +};

Structure name is not following Coding Style.

> +
> +static const VMStateDescription vmstate_icc_bus = {
> +    .name = "icc_bus",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_save = NULL,
> +    .post_load = NULL,
> +};
> +
> +struct icc_bus *g_iccbus;
> +
> +struct icc_bus *icc_init_bus(DeviceState *parent, const char *name)
> +{
> +    struct icc_bus *bus;
> +
> +    bus = FROM_QBUS(icc_bus, qbus_create(&icc_info.qinfo, parent, name));
> +    bus->qbus.allow_hotplug = 1; /* Yes, we can */
> +    bus->qbus.name = "icc";
> +    vmstate_register(NULL, -1,&vmstate_icc_bus, bus);
> +    g_iccbus = bus;
> +    return bus;
> +}
> +
> +
> +
> +
> +
> +static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
> +{
> +    SysBusDeviceInfo *info = container_of(base, SysBusDeviceInfo, qdev);
> +
> +    return info->init(sysbus_from_qdev(dev));
> +}
> +
> +void iccbus_register(SysBusDeviceInfo *info)
> +{
> +    info->qdev.init = iccbus_device_init;
> +    info->qdev.bus_info =&icc_info.qinfo;
> +
> +    assert(info->qdev.size>= sizeof(SysBusDevice));
> +    qdev_register(&info->qdev);
> +}
> +

It's not obvious to me why you need the g_iccbus variable.

> +#endif
> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> new file mode 100644
> index 0000000..94d9242
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,24 @@
> +#ifndef QEMU_ICC_H
> +#define QEMU_ICC_H


Needs copyright and a blurb explaining what this file is and what the ICC bus is.

Same comments re: whitespace below.

Regards,

Anthony Liguori

> +#include "qdev.h"
> +#include "sysbus.h"
> +
> +typedef struct icc_bus icc_bus;
> +typedef struct icc_bus_info icc_bus_info;
> +
> +
> +struct icc_bus {
> +    BusState qbus;
> +};
> +
> +struct icc_bus_info {
> +    BusInfo qinfo;
> +};
> +
> +extern struct icc_bus_info icc_info;
> +extern struct icc_bus *g_iccbus;
> +extern struct icc_bus *icc_init_bus(DeviceState *parent, const char *name);
> +extern void iccbus_register(SysBusDeviceInfo *info);

Functions don't need an 'extern' modifier.  I don't think icc_info needs to be 
exported either.

Instead of exporting g_iccbus, it would be better to have a get_icc_bus() method 
that returned the global iccbus.  That way we can more easily hook accesses to 
the bus.

> +#endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 6b3662e..10371d8 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -24,6 +24,7 @@
>   #include "hw.h"
>   #include "pc.h"
>   #include "apic.h"
> +#include "icc_bus.h"
>   #include "fdc.h"
>   #include "ide.h"
>   #include "pci.h"
> @@ -91,6 +92,7 @@ struct e820_table {
>   static struct e820_table e820_table;
>   struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
> +
>   void isa_irq_handler(void *opaque, int n, int level)
>   {
>       IsaIrqState *isa = (IsaIrqState *)opaque;
> @@ -872,13 +874,13 @@ DeviceState *cpu_get_current_apic(void)
>       }
>   }
>
> -static DeviceState *apic_init(void *env, uint8_t apic_id)
> +DeviceState *apic_init(void *env, uint8_t apic_id)
>   {
>       DeviceState *dev;
>       SysBusDevice *d;
>       static int apic_mapped;
>
> -    dev = qdev_create(NULL, "apic");
> +    dev = qdev_create(&g_iccbus->qbus, "apic");
>       qdev_prop_set_uint8(dev, "id", apic_id);
>       qdev_prop_set_ptr(dev, "cpu_env", env);
>       qdev_init_nofail(dev);
> @@ -943,10 +945,6 @@ CPUState *pc_new_cpu(const char *cpu_model)
>           fprintf(stderr, "Unable to find x86 CPU definition\n");
>           exit(1);
>       }
> -    if ((env->cpuid_features&  CPUID_APIC) || smp_cpus>  1) {
> -        env->cpuid_apic_id = env->cpu_index;
> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
> -    }
>       qemu_register_reset(pc_cpu_reset, env);
>       pc_cpu_reset(env);
>       return env;
> @@ -956,6 +954,7 @@ void pc_cpus_init(const char *cpu_model)
>   {
>       int i;
>
> +    icc_init_bus(NULL, "icc");
>       /* init CPUs */
>       for(i = 0; i<  smp_cpus; i++) {
>           pc_new_cpu(cpu_model);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 2a071f2..0160c55 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1062,4 +1062,5 @@ void svm_check_intercept(CPUState *env1, uint32_t type);
>
>   uint32_t cpu_cc_compute_all(CPUState *env1, int op);
>
> +extern DeviceState *apic_init(void *env, uint8_t apic_id);
>   #endif /* CPU_I386_H */
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 5df40d4..551a8a2 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -30,6 +30,7 @@
>   #include "monitor.h"
>   #endif
>
> +
>   //#define DEBUG_MMU
>
>   /* NOTE: must be called outside the CPU execute loop */
> @@ -1257,7 +1258,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>           return NULL;
>       }
>       mce_init(env);
> -
> +    if ((env->cpuid_features&  CPUID_APIC)
> +        || smp_cpus>  1) {
> +        env->cpuid_apic_id = env->cpu_index;
> +        env->apic_state = apic_init(env, env->cpuid_apic_id);
> +    }
>       qemu_init_vcpu(env);
>
>       return env;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 571d792..407dba6 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -880,7 +880,6 @@ static int kvm_put_sregs(CPUState *env)
>
>       sregs.cr8 = cpu_get_apic_tpr(env->apic_state);
>       sregs.apic_base = cpu_get_apic_base(env->apic_state);
> -
>       sregs.efer = env->efer;
>
>       return kvm_vcpu_ioctl(env, KVM_SET_SREGS,&sregs);

Re: [PATCH 2/2] LAPIC: make lapic support cpu hotplug

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 9469 bytes --]

On 2011-10-04 13:13, pingfank@linux.vnet.com wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Separate apic from qbus to icc bus which supports hotplug feature.

Modeling the ICC bus looks like a step in the right direction. The
IOAPIC could be attached to it as well to get rid of "ioapics[MAX_IOAPICS]".

> And make the creation of apic as part of cpu initialization, so
> apic's state has been ready, before setting kvm_apic.

There is no kvm-apic upstream yet, so it's hard to judge why we need
this here. If we do, this has to be a separate patch. But I seriously
doubt we need it (my hack worked without it, and that was not because of
its hack nature).

> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  Makefile.target      |    1 +
>  hw/apic.c            |    7 ++++-
>  hw/apic.h            |    1 +
>  hw/icc_bus.c         |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/icc_bus.h         |   24 +++++++++++++++++++
>  hw/pc.c              |   11 ++++-----
>  target-i386/cpu.h    |    1 +
>  target-i386/helper.c |    7 +++++-
>  target-i386/kvm.c    |    1 -
>  9 files changed, 105 insertions(+), 10 deletions(-)
>  create mode 100644 hw/icc_bus.c
>  create mode 100644 hw/icc_bus.h
> 
> diff --git a/Makefile.target b/Makefile.target
> index 9011f28..5607c6d 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>  obj-i386-y += testdev.o
>  obj-i386-y += acpi.o acpi_piix4.o
> +obj-i386-y += icc_bus.o
>  
>  obj-i386-y += pcspk.o i8254.o
>  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> diff --git a/hw/apic.c b/hw/apic.c
> index 69d6ac5..95a1664 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -24,6 +24,7 @@
>  #include "sysbus.h"
>  #include "trace.h"
>  #include "kvm.h"
> +#include "icc_bus.h"
>  
>  /* APIC Local Vector Table */
>  #define APIC_LVT_TIMER   0
> @@ -304,11 +305,13 @@ void cpu_set_apic_base(DeviceState *d, uint64_t val)
>  
>      if (!s)
>          return;
> +
>      if (kvm_enabled() && kvm_irqchip_in_kernel())
>          s->apicbase = val;
>      else
>          s->apicbase = (val & 0xfffff000) |
>              (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE));
> +
>      /* if disabled, cannot be enabled again */
>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
>          s->apicbase &= ~MSR_IA32_APICBASE_ENABLE;
> @@ -1075,7 +1078,7 @@ static const VMStateDescription vmstate_apic = {
>      }
>  };
>  
> -static void apic_reset(DeviceState *d)
> +void apic_reset(DeviceState *d)

Still unused outside of this file, so keep it private.

>  {
>      APICState *s = DO_UPCAST(APICState, busdev.qdev, d);
>      int bsp;
> @@ -1138,7 +1141,7 @@ static SysBusDeviceInfo apic_info = {
>  
>  static void apic_register_devices(void)
>  {
> -    sysbus_register_withprop(&apic_info);
> +    iccbus_register(&apic_info);
>  }
>  
>  device_init(apic_register_devices)
> diff --git a/hw/apic.h b/hw/apic.h
> index c857d52..e258efa 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -24,5 +24,6 @@ void apic_sipi(DeviceState *s);
>  /* pc.c */
>  int cpu_is_bsp(CPUState *env);
>  DeviceState *cpu_get_current_apic(void);
> +void apic_reset(DeviceState *d);
>  
>  #endif
> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> new file mode 100644
> index 0000000..360ca2a
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,62 @@
> +/*
> +*/
> +#define ICC_BUS_PLUG
> +#ifdef ICC_BUS_PLUG
> +#include "icc_bus.h"
> +
> +
> +
> +struct icc_bus_info icc_info = {
> +    .qinfo.name = "icc",
> +    .qinfo.size = sizeof(struct icc_bus),
> +    .qinfo.props = (Property[]) {
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +
> +};
> +
> +
> +static const VMStateDescription vmstate_icc_bus = {
> +    .name = "icc_bus",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_save = NULL,
> +    .post_load = NULL,
> +};
> +
> +struct icc_bus *g_iccbus;
> +
> +struct icc_bus *icc_init_bus(DeviceState *parent, const char *name)
> +{
> +    struct icc_bus *bus;
> +
> +    bus = FROM_QBUS(icc_bus, qbus_create(&icc_info.qinfo, parent, name));
> +    bus->qbus.allow_hotplug = 1; /* Yes, we can */
> +    bus->qbus.name = "icc";
> +    vmstate_register(NULL, -1, &vmstate_icc_bus, bus);

The chipset is the owner of this bus and instantiates it. So it also
provides a vmstate. You can drop this unneeded one here (it's created
via an obsolete API anyway).

> +    g_iccbus = bus;
> +    return bus;
> +}
> +
> +
> +
> +
> +
> +static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
> +{
> +    SysBusDeviceInfo *info = container_of(base, SysBusDeviceInfo, qdev);
> +
> +    return info->init(sysbus_from_qdev(dev));
> +}
> +
> +void iccbus_register(SysBusDeviceInfo *info)
> +{
> +    info->qdev.init = iccbus_device_init;
> +    info->qdev.bus_info = &icc_info.qinfo;
> +
> +    assert(info->qdev.size >= sizeof(SysBusDevice));
> +    qdev_register(&info->qdev);
> +}

This service should be unneeded when the bus is properly embedded into
its parent (ie. the chipset).

> +
> +#endif
> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> new file mode 100644
> index 0000000..94d9242
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,24 @@
> +#ifndef QEMU_ICC_H
> +#define QEMU_ICC_H
> +
> +#include "qdev.h"
> +#include "sysbus.h"
> +
> +typedef struct icc_bus icc_bus;
> +typedef struct icc_bus_info icc_bus_info;
> +
> +
> +struct icc_bus {
> +    BusState qbus;
> +};
> +
> +struct icc_bus_info {
> +    BusInfo qinfo;
> +};
> +
> +extern struct icc_bus_info icc_info;
> +extern struct icc_bus *g_iccbus;
> +extern struct icc_bus *icc_init_bus(DeviceState *parent, const char *name);
> +extern void iccbus_register(SysBusDeviceInfo *info);
> +
> +#endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 6b3662e..10371d8 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -24,6 +24,7 @@
>  #include "hw.h"
>  #include "pc.h"
>  #include "apic.h"
> +#include "icc_bus.h"
>  #include "fdc.h"
>  #include "ide.h"
>  #include "pci.h"
> @@ -91,6 +92,7 @@ struct e820_table {
>  static struct e820_table e820_table;
>  struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  
> +
>  void isa_irq_handler(void *opaque, int n, int level)
>  {
>      IsaIrqState *isa = (IsaIrqState *)opaque;
> @@ -872,13 +874,13 @@ DeviceState *cpu_get_current_apic(void)
>      }
>  }
>  
> -static DeviceState *apic_init(void *env, uint8_t apic_id)
> +DeviceState *apic_init(void *env, uint8_t apic_id)
>  {
>      DeviceState *dev;
>      SysBusDevice *d;
>      static int apic_mapped;
>  
> -    dev = qdev_create(NULL, "apic");
> +    dev = qdev_create(&g_iccbus->qbus, "apic");
>      qdev_prop_set_uint8(dev, "id", apic_id);
>      qdev_prop_set_ptr(dev, "cpu_env", env);
>      qdev_init_nofail(dev);
> @@ -943,10 +945,6 @@ CPUState *pc_new_cpu(const char *cpu_model)
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          exit(1);
>      }
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->cpuid_apic_id = env->cpu_index;
> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
> -    }
>      qemu_register_reset(pc_cpu_reset, env);
>      pc_cpu_reset(env);
>      return env;
> @@ -956,6 +954,7 @@ void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
>  
> +    icc_init_bus(NULL, "icc");
>      /* init CPUs */
>      for(i = 0; i < smp_cpus; i++) {
>          pc_new_cpu(cpu_model);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 2a071f2..0160c55 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1062,4 +1062,5 @@ void svm_check_intercept(CPUState *env1, uint32_t type);
>  
>  uint32_t cpu_cc_compute_all(CPUState *env1, int op);
>  
> +extern DeviceState *apic_init(void *env, uint8_t apic_id);
>  #endif /* CPU_I386_H */
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 5df40d4..551a8a2 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -30,6 +30,7 @@
>  #include "monitor.h"
>  #endif
>  
> +
>  //#define DEBUG_MMU
>  
>  /* NOTE: must be called outside the CPU execute loop */
> @@ -1257,7 +1258,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>          return NULL;
>      }
>      mce_init(env);
> -
> +    if ((env->cpuid_features & CPUID_APIC)
> +        || smp_cpus > 1) {
> +        env->cpuid_apic_id = env->cpu_index;
> +        env->apic_state = apic_init(env, env->cpuid_apic_id);
> +    }
>      qemu_init_vcpu(env);
>  
>      return env;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 571d792..407dba6 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -880,7 +880,6 @@ static int kvm_put_sregs(CPUState *env)
>  
>      sregs.cr8 = cpu_get_apic_tpr(env->apic_state);
>      sregs.apic_base = cpu_get_apic_base(env->apic_state);
> -
>      sregs.efer = env->efer;
>  
>      return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);

Would be good to see the full series, i.e. everything that is required
to make CPU hotplug work. First for QEMU upstream without in-kernel
irqchips and then the qemu-kvm bits.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 2/2] LAPIC: make lapic support cpu hotplug

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3453 bytes --]

On 2011-10-05 12:26, liu ping fan wrote:
>>  > And make the creation of apic as part of cpu initialization, so
>>> apic's state has been ready, before setting kvm_apic.
>>
>> There is no kvm-apic upstream yet, so it's hard to judge why we need
>> this here. If we do, this has to be a separate patch. But I seriously
>> doubt we need it (my hack worked without it, and that was not because of
>> its hack nature).
>>
>> Sorry, I did not explain it clearly. What I mean is that “env->apic_state”
> must be prepared
> before qemu_kvm_cpu_thread_fn() -> ... -> kvm_put_sregs(), where we get
> apic_base by
> “ sregs.apic_base = cpu_get_apic_base(env->apic_state);”
> and then call “kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);” which will
> finally affect the
> kvm_apic structure in kernel.
> 
> But as current code, in pc_new_cpu(), we call apic_init() to initialize
> apic_state, after cpu_init(),
> so we can not guarantee the order of apic_state initializaion and the
> setting to kernel.
> 
> Because LAPIC is part of x86 chip, I want to move it into cpu_x86_init(),
> and ensure apic_init()
> called before thread “qemu_kvm_cpu_thread_fn()” creation.

The LAPIC is part of the CPU, the classic APIC was a dedicated chip.

For various reasons, a safer approach for creating a new CPU is to stop
the machine, add the new device models, run cpu_synchronize_post_init on
that new cpu (looks like you missed that) and then resume everything.
See
http://git.kiszka.org/?p=qemu-kvm.git;a=commitdiff;h=be8f21c6b54eac82f7add7ee9d4ecf9cb8ebb320

...
>>> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
>>> new file mode 100644
>>> index 0000000..360ca2a
>>> --- /dev/null
>>> +++ b/hw/icc_bus.c
>>> @@ -0,0 +1,62 @@
>>> +/*
>>> +*/
>>> +#define ICC_BUS_PLUG
>>> +#ifdef ICC_BUS_PLUG
>>> +#include "icc_bus.h"
>>> +
>>> +
>>> +
>>> +struct icc_bus_info icc_info = {
>>> +    .qinfo.name = "icc",
>>> +    .qinfo.size = sizeof(struct icc_bus),
>>> +    .qinfo.props = (Property[]) {
>>> +        DEFINE_PROP_END_OF_LIST(),
>>> +    }
>>> +
>>> +};
>>> +
>>> +
>>> +static const VMStateDescription vmstate_icc_bus = {
>>> +    .name = "icc_bus",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .minimum_version_id_old = 1,
>>> +    .pre_save = NULL,
>>> +    .post_load = NULL,
>>> +};
>>> +
>>> +struct icc_bus *g_iccbus;
>>> +
>>> +struct icc_bus *icc_init_bus(DeviceState *parent, const char *name)
>>> +{
>>> +    struct icc_bus *bus;
>>> +
>>> +    bus = FROM_QBUS(icc_bus, qbus_create(&icc_info.qinfo, parent,
>> name));
>>> +    bus->qbus.allow_hotplug = 1; /* Yes, we can */
>>> +    bus->qbus.name = "icc";
>>> +    vmstate_register(NULL, -1, &vmstate_icc_bus, bus);
>>
>> The chipset is the owner of this bus and instantiates it. So it also
>> provides a vmstate. You can drop this unneeded one here (it's created
>> via an obsolete API anyway).
>>
> 
> No familiar with Qemu bus emulation, keep on learning :) . But what I
> thought is,
> the x86-ICC bus is not the same as bus like PCI.
> For a PCI bus, it lies behind a host bridge, but ICC is shared by all x86
> processors in SMP system,
> so there is not a outstanding owner.  And I right?

ICC is also attached to the chipset (due to the IOAPIC). So it looks
reasonable to me to let the chipset do the lifecycle management as well.
It is the fixed point, CPUs may come and go.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]