| Message ID | 1329347774-23262-3-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2012-02-16 00:16, Igor Mammedov wrote: > Convert pc cpu to qdev device that is attached to icc bus, later > hot-plug ability of icc bus will allow to implement cpu hot-plug. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/pc.c | 62 +++++++++++++++++++++++++++++++++++++++++++------ > target-i386/cpu.h | 1 + > target-i386/helper.c | 26 ++++++++++++++------ > 3 files changed, 73 insertions(+), 16 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 33d8090..b8db5dc 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > } > } > > +typedef struct CPUPC { > + ICCBusDevice busdev; > + char *model; > + CPUState state; > +} CPUPC; > + > static void pc_cpu_reset(void *opaque) > { > CPUState *env = opaque; > @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque) > env->halted = !cpu_is_bsp(env); > } > > -static CPUState *pc_new_cpu(const char *cpu_model) > +static DeviceState *pc_new_cpu(const char *cpu_model) > { > - CPUState *env; > + DeviceState *dev; > + BusState *b; > > - env = cpu_init(cpu_model); > - if (!env) { > - fprintf(stderr, "Unable to find x86 CPU definition\n"); > - exit(1); > + b = get_icc_bus(); > + dev = qdev_create(b, "cpu-pc"); > + if (!dev) { > + return NULL; > + } > + qdev_prop_set_string(dev, "model", g_strdup(cpu_model)); > + if (qdev_init(dev) < 0) { > + return NULL; > + } > + return dev; > +} > + > +static int cpu_device_init(ICCBusDevice *dev) > +{ > + CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev); > + CPUState *env = &cpu->state; > + > + if (cpu_x86_init_inplace(env, cpu->model) < 0) { > + return -1; > } > + > if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > - qemu_register_reset(pc_cpu_reset, env); > + > + return 0; > +} > + > +static void cpu_device_reset(DeviceState *dev) { > + CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev); > + CPUState *env = &cpu->state; > pc_cpu_reset(env); > - return env; > } > > +static ICCBusDeviceInfo cpu_device_info = { > + .qdev.name = "cpu-pc", > + .qdev.size = sizeof(CPUPC), > + .qdev.reset = cpu_device_reset, > + .init = cpu_device_init, > + .qdev.props = (Property[]) { > + DEFINE_PROP_STRING("model", CPUPC, model), > + DEFINE_PROP_END_OF_LIST(), > + }, > +}; > + > +static void pc_register_devices(void) > +{ > + iccbus_register_devinfo(&cpu_device_info); > +} > + > +device_init(pc_register_devices); > + > void pc_cpus_init(const char *cpu_model) > { > int i; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 7e66bcf..830b65e 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -775,6 +775,7 @@ typedef struct CPUX86State { > } CPUX86State; > > CPUX86State *cpu_x86_init(const char *cpu_model); > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model); > int cpu_x86_exec(CPUX86State *s); > void cpu_x86_close(CPUX86State *s); > void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg); > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 2586aff..df2f5ba 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, > return 1; > } > > -CPUX86State *cpu_x86_init(const char *cpu_model) > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model) > { > - CPUX86State *env; > static int inited; > > - env = g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_register(env, cpu_model) < 0) { > + fprintf(stderr, "Unable to find x86 CPU definition\n"); > + return -1; > + } > cpu_exec_init(env); > env->cpu_model_str = cpu_model; > > @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > cpu_set_debug_excp_handler(breakpoint_handler); > #endif > } > - if (cpu_x86_register(env, cpu_model) < 0) { > - cpu_x86_close(env); > - return NULL; > - } > env->cpuid_apic_id = env->cpu_index; > mce_init(env); > > qemu_init_vcpu(env); > > - return env; > + return 0; > } > > +CPUX86State *cpu_x86_init(const char *cpu_model) > +{ > + CPUX86State *env; > + > + env = g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_init_inplace(env, cpu_model) < 0) { > + g_free(env); > + return NULL; > + } > + > + return env; > +} > #if !defined(CONFIG_USER_ONLY) > void do_cpu_init(CPUState *env) > { QOM is required here as well. Andreas is also working on CPU devices. I guess you should coordinate efforts. Jan
On 2012-02-16 00:16, Igor Mammedov wrote: > +static ICCBusDeviceInfo cpu_device_info = { > + .qdev.name = "cpu-pc", > + .qdev.size = sizeof(CPUPC), > + .qdev.reset = cpu_device_reset, > + .init = cpu_device_init, > + .qdev.props = (Property[]) { > + DEFINE_PROP_STRING("model", CPUPC, model), And how do you pass in feature flags? Or the core layout? Basically both -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" (not "pc") commands. As a shortcut, "-smp N" would continue to replicate a "-device cpu-XX" N times. Otherwise, configuration becomes inconsistent. Jan
On 02/15/2012 05:16 PM, Igor Mammedov wrote: > Convert pc cpu to qdev device that is attached to icc bus, later > hot-plug ability of icc bus will allow to implement cpu hot-plug. > > Signed-off-by: Igor Mammedov<imammedo@redhat.com> Obviously, this needs to go to QOM, but also, this is a shallow conversion. Beginning with a shallow conversion is okay but only if there's a clear path to a proper conversion. I don't think this approach fits that requirement. The approach is a bit too pc centric. It's not clear how it would generalize. Regards, Anthony Liguori > --- > hw/pc.c | 62 +++++++++++++++++++++++++++++++++++++++++++------ > target-i386/cpu.h | 1 + > target-i386/helper.c | 26 ++++++++++++++------ > 3 files changed, 73 insertions(+), 16 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 33d8090..b8db5dc 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > } > } > > +typedef struct CPUPC { > + ICCBusDevice busdev; > + char *model; > + CPUState state; > +} CPUPC; > + > static void pc_cpu_reset(void *opaque) > { > CPUState *env = opaque; > @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque) > env->halted = !cpu_is_bsp(env); > } > > -static CPUState *pc_new_cpu(const char *cpu_model) > +static DeviceState *pc_new_cpu(const char *cpu_model) > { > - CPUState *env; > + DeviceState *dev; > + BusState *b; > > - env = cpu_init(cpu_model); > - if (!env) { > - fprintf(stderr, "Unable to find x86 CPU definition\n"); > - exit(1); > + b = get_icc_bus(); > + dev = qdev_create(b, "cpu-pc"); > + if (!dev) { > + return NULL; > + } > + qdev_prop_set_string(dev, "model", g_strdup(cpu_model)); > + if (qdev_init(dev)< 0) { > + return NULL; > + } > + return dev; > +} > + > +static int cpu_device_init(ICCBusDevice *dev) > +{ > + CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev); > + CPUState *env =&cpu->state; > + > + if (cpu_x86_init_inplace(env, cpu->model)< 0) { > + return -1; > } > + > if ((env->cpuid_features& CPUID_APIC) || smp_cpus> 1) { > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > - qemu_register_reset(pc_cpu_reset, env); > + > + return 0; > +} > + > +static void cpu_device_reset(DeviceState *dev) { > + CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev); > + CPUState *env =&cpu->state; > pc_cpu_reset(env); > - return env; > } > > +static ICCBusDeviceInfo cpu_device_info = { > + .qdev.name = "cpu-pc", > + .qdev.size = sizeof(CPUPC), > + .qdev.reset = cpu_device_reset, > + .init = cpu_device_init, > + .qdev.props = (Property[]) { > + DEFINE_PROP_STRING("model", CPUPC, model), > + DEFINE_PROP_END_OF_LIST(), > + }, > +}; > + > +static void pc_register_devices(void) > +{ > + iccbus_register_devinfo(&cpu_device_info); > +} > + > +device_init(pc_register_devices); > + > void pc_cpus_init(const char *cpu_model) > { > int i; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 7e66bcf..830b65e 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -775,6 +775,7 @@ typedef struct CPUX86State { > } CPUX86State; > > CPUX86State *cpu_x86_init(const char *cpu_model); > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model); > int cpu_x86_exec(CPUX86State *s); > void cpu_x86_close(CPUX86State *s); > void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg); > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 2586aff..df2f5ba 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, > return 1; > } > > -CPUX86State *cpu_x86_init(const char *cpu_model) > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model) > { > - CPUX86State *env; > static int inited; > > - env = g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_register(env, cpu_model)< 0) { > + fprintf(stderr, "Unable to find x86 CPU definition\n"); > + return -1; > + } > cpu_exec_init(env); > env->cpu_model_str = cpu_model; > > @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > cpu_set_debug_excp_handler(breakpoint_handler); > #endif > } > - if (cpu_x86_register(env, cpu_model)< 0) { > - cpu_x86_close(env); > - return NULL; > - } > env->cpuid_apic_id = env->cpu_index; > mce_init(env); > > qemu_init_vcpu(env); > > - return env; > + return 0; > } > > +CPUX86State *cpu_x86_init(const char *cpu_model) > +{ > + CPUX86State *env; > + > + env = g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_init_inplace(env, cpu_model)< 0) { > + g_free(env); > + return NULL; > + } > + > + return env; > +} > #if !defined(CONFIG_USER_ONLY) > void do_cpu_init(CPUState *env) > {
On 02/16/2012 06:01 AM, Jan Kiszka wrote: > On 2012-02-16 00:16, Igor Mammedov wrote: >> +static ICCBusDeviceInfo cpu_device_info = { >> + .qdev.name = "cpu-pc", >> + .qdev.size = sizeof(CPUPC), >> + .qdev.reset = cpu_device_reset, >> + .init = cpu_device_init, >> + .qdev.props = (Property[]) { >> + DEFINE_PROP_STRING("model", CPUPC, model), > > And how do you pass in feature flags? Or the core layout? Basically both > -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" > (not "pc") commands. The approach that I'd recommend is: 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately. 2) convert as many users of CPUState to CPUCommonState as possible. 3) make CPUCommonState a base class, move it to the front of the structure, and attempt to measure the impact to TCG. 4) if (3) is significant, special case things in QOM 5) make target specific code use target specific CPUState typename 6) eliminate #define CPUState 7) make on-processor devices (like lapic) children of target specific CPUState 8) express target specific flags/features via QOM properties 9) make machine expose target specific CPUState links that can be set by the user. Regards, Anthony Liguori > As a shortcut, "-smp N" would continue to replicate > a "-device cpu-XX" N times. Otherwise, configuration becomes inconsistent. > > Jan >
On 2012-02-16 13:51, Anthony Liguori wrote: > On 02/16/2012 06:01 AM, Jan Kiszka wrote: >> On 2012-02-16 00:16, Igor Mammedov wrote: >>> +static ICCBusDeviceInfo cpu_device_info = { >>> + .qdev.name = "cpu-pc", >>> + .qdev.size = sizeof(CPUPC), >>> + .qdev.reset = cpu_device_reset, >>> + .init = cpu_device_init, >>> + .qdev.props = (Property[]) { >>> + DEFINE_PROP_STRING("model", CPUPC, model), >> >> And how do you pass in feature flags? Or the core layout? Basically both >> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" >> (not "pc") commands. > > The approach that I'd recommend is: > > 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace > all references to members of CPU_COMMON_STATE appropriately. > > 2) convert as many users of CPUState to CPUCommonState as possible. > > 3) make CPUCommonState a base class, move it to the front of the structure, and > attempt to measure the impact to TCG. > > 4) if (3) is significant, special case things in QOM > > 5) make target specific code use target specific CPUState typename > > 6) eliminate #define CPUState > > 7) make on-processor devices (like lapic) children of target specific CPUState > > 8) express target specific flags/features via QOM properties > > 9) make machine expose target specific CPUState links that can be set by the user. Also, I'm wondering what names those CPUs should have. I think we should expose model names as device names and avoid a "model" property. Jan
Am 16.02.2012 00:16, schrieb Igor Mammedov: > Convert pc cpu to qdev device that is attached to icc bus, later > hot-plug ability of icc bus will allow to implement cpu hot-plug. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> This conflicts with CPU QOM'ification across targets (and no longer applies due to type_init() introduction). > --- > hw/pc.c | 62 +++++++++++++++++++++++++++++++++++++++++++------ > target-i386/cpu.h | 1 + > target-i386/helper.c | 26 ++++++++++++++------ > 3 files changed, 73 insertions(+), 16 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 33d8090..b8db5dc 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > } > } > > +typedef struct CPUPC { > + ICCBusDevice busdev; > + char *model; > + CPUState state; > +} CPUPC; I don't like this approach. For starters, using CPUState as field rather than pointer seems a bad idea to me (CPUX86State would only be slightly better). We should have a single code path through which we instantiate CPUs, currently: cpu_$arch_init(const char *cpu_model) With my series completed, this would return an X86CPU object. Depending on our liking, we could either place some ICC / APIC / whatever fields directly into that, or embed the X86CPU in an object such as yours above as a link<X86CPU>. I do feel however that the model string is misplaced there. Question is whether this ICC stuff is actually part of the CPU or part of the CPU wiring on the mainboard - I vaguely remember someone saying that this changed over time...? Having both depending on CPU subclass might also be an option, but I'd rather leave such decisions as a follow-up to the core QOM'ification. Andreas > + > static void pc_cpu_reset(void *opaque) > { > CPUState *env = opaque; > @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque) > env->halted = !cpu_is_bsp(env); > } > > -static CPUState *pc_new_cpu(const char *cpu_model) > +static DeviceState *pc_new_cpu(const char *cpu_model) > { > - CPUState *env; > + DeviceState *dev; > + BusState *b; > > - env = cpu_init(cpu_model); > - if (!env) { > - fprintf(stderr, "Unable to find x86 CPU definition\n"); > - exit(1); > + b = get_icc_bus(); > + dev = qdev_create(b, "cpu-pc"); > + if (!dev) { > + return NULL; > + } > + qdev_prop_set_string(dev, "model", g_strdup(cpu_model)); > + if (qdev_init(dev) < 0) { > + return NULL; > + } > + return dev; > +} > + > +static int cpu_device_init(ICCBusDevice *dev) > +{ > + CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev); > + CPUState *env = &cpu->state; > + > + if (cpu_x86_init_inplace(env, cpu->model) < 0) { > + return -1; > } > + > if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > - qemu_register_reset(pc_cpu_reset, env); > + > + return 0; > +} > + > +static void cpu_device_reset(DeviceState *dev) { > + CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev); > + CPUState *env = &cpu->state; > pc_cpu_reset(env); > - return env; > } > > +static ICCBusDeviceInfo cpu_device_info = { > + .qdev.name = "cpu-pc", > + .qdev.size = sizeof(CPUPC), > + .qdev.reset = cpu_device_reset, > + .init = cpu_device_init, > + .qdev.props = (Property[]) { > + DEFINE_PROP_STRING("model", CPUPC, model), > + DEFINE_PROP_END_OF_LIST(), > + }, > +}; > + > +static void pc_register_devices(void) > +{ > + iccbus_register_devinfo(&cpu_device_info); > +} > + > +device_init(pc_register_devices); > + > void pc_cpus_init(const char *cpu_model) > { > int i; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 7e66bcf..830b65e 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -775,6 +775,7 @@ typedef struct CPUX86State { > } CPUX86State; > > CPUX86State *cpu_x86_init(const char *cpu_model); > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model); > int cpu_x86_exec(CPUX86State *s); > void cpu_x86_close(CPUX86State *s); > void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg); > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 2586aff..df2f5ba 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, > return 1; > } > > -CPUX86State *cpu_x86_init(const char *cpu_model) > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model) > { > - CPUX86State *env; > static int inited; > > - env = g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_register(env, cpu_model) < 0) { > + fprintf(stderr, "Unable to find x86 CPU definition\n"); > + return -1; > + } > cpu_exec_init(env); > env->cpu_model_str = cpu_model; > > @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > cpu_set_debug_excp_handler(breakpoint_handler); > #endif > } > - if (cpu_x86_register(env, cpu_model) < 0) { > - cpu_x86_close(env); > - return NULL; > - } > env->cpuid_apic_id = env->cpu_index; > mce_init(env); > > qemu_init_vcpu(env); > > - return env; > + return 0; > } > > +CPUX86State *cpu_x86_init(const char *cpu_model) > +{ > + CPUX86State *env; > + > + env = g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_init_inplace(env, cpu_model) < 0) { > + g_free(env); > + return NULL; > + } > + > + return env; > +} > #if !defined(CONFIG_USER_ONLY) > void do_cpu_init(CPUState *env) > {
On 02/16/2012 06:54 AM, Jan Kiszka wrote: > On 2012-02-16 13:51, Anthony Liguori wrote: >> On 02/16/2012 06:01 AM, Jan Kiszka wrote: >>> On 2012-02-16 00:16, Igor Mammedov wrote: >>>> +static ICCBusDeviceInfo cpu_device_info = { >>>> + .qdev.name = "cpu-pc", >>>> + .qdev.size = sizeof(CPUPC), >>>> + .qdev.reset = cpu_device_reset, >>>> + .init = cpu_device_init, >>>> + .qdev.props = (Property[]) { >>>> + DEFINE_PROP_STRING("model", CPUPC, model), >>> >>> And how do you pass in feature flags? Or the core layout? Basically both >>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" >>> (not "pc") commands. >> >> The approach that I'd recommend is: >> >> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace >> all references to members of CPU_COMMON_STATE appropriately. >> >> 2) convert as many users of CPUState to CPUCommonState as possible. >> >> 3) make CPUCommonState a base class, move it to the front of the structure, and >> attempt to measure the impact to TCG. >> >> 4) if (3) is significant, special case things in QOM >> >> 5) make target specific code use target specific CPUState typename >> >> 6) eliminate #define CPUState >> >> 7) make on-processor devices (like lapic) children of target specific CPUState >> >> 8) express target specific flags/features via QOM properties >> >> 9) make machine expose target specific CPUState links that can be set by the user. > > Also, I'm wondering what names those CPUs should have. I think we should > expose model names as device names and avoid a "model" property. Yeah, I think we want a CPUX86State and then a bunch of subclasses defined by that. By making use of the class_data field, we can register classes based on combinations of feature flags. So for instance: static TypeInfo opteron_g2 = { .name = "cpu-opteron-g2", .parent = TYPE_CPU_X86", .class_init = cpu_x86_generic_class_init, .class_data = (CPUX86Definition[]){ .level = 5, .vendor = "AuthenticAMD", .family = 15, .model = 6, .stepping = 1, .feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu mtrr clflush mca pse36", ... }, }; And obviously, we can still using the existing cpudef directive to generate types. Regards, Anthony Liguori > > Jan >
On 02/16/2012 05:09 PM, Andreas Färber wrote: > Am 16.02.2012 00:16, schrieb Igor Mammedov: >> Convert pc cpu to qdev device that is attached to icc bus, later >> hot-plug ability of icc bus will allow to implement cpu hot-plug. >> >> Signed-off-by: Igor Mammedov<imammedo@redhat.com> > > This conflicts with CPU QOM'ification across targets (and no longer > applies due to type_init() introduction). > >> --- >> hw/pc.c | 62 +++++++++++++++++++++++++++++++++++++++++++------ >> target-i386/cpu.h | 1 + >> target-i386/helper.c | 26 ++++++++++++++------ >> 3 files changed, 73 insertions(+), 16 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 33d8090..b8db5dc 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) >> } >> } >> >> +typedef struct CPUPC { >> + ICCBusDevice busdev; >> + char *model; >> + CPUState state; >> +} CPUPC; > > I don't like this approach. For starters, using CPUState as field rather > than pointer seems a bad idea to me (CPUX86State would only be slightly > better). We should have a single code path through which we instantiate > CPUs, currently: cpu_$arch_init(const char *cpu_model) > With my series completed, this would return an X86CPU object. > > Depending on our liking, we could either place some ICC / APIC / > whatever fields directly into that, or embed the X86CPU in an object > such as yours above as a link<X86CPU>. I do feel however that the model > string is misplaced there. Question is whether this ICC stuff is > actually part of the CPU or part of the CPU wiring on the mainboard - I > vaguely remember someone saying that this changed over time...? Having Yep, since P4 times sysbus used instead of icc so we can just ignore icc. > both depending on CPU subclass might also be an option, but I'd rather > leave such decisions as a follow-up to the core QOM'ification. > With QOM and your work this patch is obsolete. I see you've already QOM-ified X86CPU in your qom-cpu tree. With your permission I'll play with it and check what could be done for cpu hot-plug feature.
Am 17.02.2012 18:16, schrieb Igor Mammedov: > On 02/16/2012 05:09 PM, Andreas Färber wrote: >> We should have a single code path through which we instantiate >> CPUs, currently: cpu_$arch_init(const char *cpu_model) >> With my series completed, this would return an X86CPU object. >> >> Depending on our liking, we could either place some ICC / APIC / >> whatever fields directly into that, or embed the X86CPU in an object >> such as yours above as a link<X86CPU>. I do feel however that the model >> string is misplaced there. Question is whether this ICC stuff is >> actually part of the CPU or part of the CPU wiring on the mainboard - I >> vaguely remember someone saying that this changed over time...? Having > Yep, since P4 times sysbus used instead of icc so we can just ignore icc. > >> both depending on CPU subclass might also be an option, but I'd rather >> leave such decisions as a follow-up to the core QOM'ification. >> > > With QOM and your work this patch is obsolete. I see you've already > QOM-ified X86CPU in your qom-cpu tree. Yeah, I postponed PowerPC and took a shortcut by renaming cpuid.c. ;) > With your permission I'll play with it and > check > what could be done for cpu hot-plug feature. Sure, just beware that I frequently rebase this branch based on feedback or moving upstream. Also note that the CPUArchState refactoring is meant only temporary (I'm using a script for that) and should be avoided in new code in favor of CPUX86State or X86CPU/X86CPUClass. If you need some change to make your hotplug work easier, just let me know. Andreas
On 02/16/2012 08:51 PM, Anthony Liguori wrote: > On 02/16/2012 06:01 AM, Jan Kiszka wrote: >> On 2012-02-16 00:16, Igor Mammedov wrote: >>> +static ICCBusDeviceInfo cpu_device_info = { >>> + .qdev.name = "cpu-pc", >>> + .qdev.size = sizeof(CPUPC), >>> + .qdev.reset = cpu_device_reset, >>> + .init = cpu_device_init, >>> + .qdev.props = (Property[]) { >>> + DEFINE_PROP_STRING("model", CPUPC, model), >> >> And how do you pass in feature flags? Or the core layout? Basically both >> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" >> (not "pc") commands. > > The approach that I'd recommend is: > > 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately. > Hi, All I just tried this for several days, it will result a huge patch, it is hard for human, any suggestion? (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard, it still leaves huge part which needs manual conversion/fix) I will take part in implementing cpu-hotplug future for qemu, Could you give me some tips/suggestions? What approach should I take? The main work of my "taking part in" is assisting for this community, So you can assign some works/steps to me. Thanks, Lai
Am 13.03.2012 10:32, schrieb Lai Jiangshan: > On 02/16/2012 08:51 PM, Anthony Liguori wrote: >> On 02/16/2012 06:01 AM, Jan Kiszka wrote: >>> On 2012-02-16 00:16, Igor Mammedov wrote: >>>> +static ICCBusDeviceInfo cpu_device_info = { >>>> + .qdev.name = "cpu-pc", >>>> + .qdev.size = sizeof(CPUPC), >>>> + .qdev.reset = cpu_device_reset, >>>> + .init = cpu_device_init, >>>> + .qdev.props = (Property[]) { >>>> + DEFINE_PROP_STRING("model", CPUPC, model), >>> >>> And how do you pass in feature flags? Or the core layout? Basically both >>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" >>> (not "pc") commands. >> >> The approach that I'd recommend is: >> >> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately. > > I just tried this for several days, it will result a huge patch, > it is hard for human, any suggestion? > (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard, > it still leaves huge part which needs manual conversion/fix) > > I will take part in implementing cpu-hotplug future for qemu, > Could you give me some tips/suggestions? What approach should I take? In short: Don't! Please instead provide feedback on my series which already does something like that for you, available since multiple weeks. If a different naming is preferred, I can change it. The approach I have taken is: 1. CPUState -> CPU{X86,...}State, CPUArchState as alias where necessary 2. Embed CPU$archState in $archCPU 3. Step by step move CPU_COMMON_STATE from CPUArchState into CPUState Steps 1+2 are done, 2 is partially on the list (e.g., [1]), the rest on my qom-cpu-wip branch [2]. Step 3 is shown for icount on qom-cpu-wip as well as for multiple target-specific fields such as target-arm features [3]. Contributions welcome. Whatever naming and order we decide on, this WILL touch a lot of code. If you start doing this in some random different way we'll end up with conflicting series on the list, wasting each other's time. Thanks, Andreas [1] http://patchwork.ozlabs.org/patch/145800/ [2] http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-cpu-wip [3] http://patchwork.ozlabs.org/patch/145874/
On 03/13/2012 07:04 PM, Andreas Färber wrote: > Am 13.03.2012 10:32, schrieb Lai Jiangshan: >> On 02/16/2012 08:51 PM, Anthony Liguori wrote: >>> On 02/16/2012 06:01 AM, Jan Kiszka wrote: >>>> On 2012-02-16 00:16, Igor Mammedov wrote: >>>>> +static ICCBusDeviceInfo cpu_device_info = { >>>>> + .qdev.name = "cpu-pc", >>>>> + .qdev.size = sizeof(CPUPC), >>>>> + .qdev.reset = cpu_device_reset, >>>>> + .init = cpu_device_init, >>>>> + .qdev.props = (Property[]) { >>>>> + DEFINE_PROP_STRING("model", CPUPC, model), >>>> >>>> And how do you pass in feature flags? Or the core layout? Basically both >>>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" >>>> (not "pc") commands. >>> >>> The approach that I'd recommend is: >>> >>> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately. >> >> I just tried this for several days, it will result a huge patch, >> it is hard for human, any suggestion? >> (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard, >> it still leaves huge part which needs manual conversion/fix) >> >> I will take part in implementing cpu-hotplug future for qemu, >> Could you give me some tips/suggestions? What approach should I take? > > In short: Don't! > > Please instead provide feedback on my series which already does > something like that for you, available since multiple weeks. If a > different naming is preferred, I can change it. > > The approach I have taken is: > 1. CPUState -> CPU{X86,...}State, CPUArchState as alias where necessary > 2. Embed CPU$archState in $archCPU > 3. Step by step move CPU_COMMON_STATE from CPUArchState into CPUState > > Steps 1+2 are done, 2 is partially on the list (e.g., [1]), the rest on > my qom-cpu-wip branch [2]. > Step 3 is shown for icount on qom-cpu-wip as well as for multiple > target-specific fields such as target-arm features [3]. Contributions > welcome. OK, your work can't be done automatically, but it do be worth when there are multiple target-specific fields existed. Anthony Liguori, Jan Kiszka, Could you light me what can I do for cpu-hotplug. I saw several approaches are sent to the maillist, but they are not accepted, so I don't know how to take part in. Thanks, Lai > > Whatever naming and order we decide on, this WILL touch a lot of code. > If you start doing this in some random different way we'll end up with > conflicting series on the list, wasting each other's time. > > Thanks, > Andreas > > [1] http://patchwork.ozlabs.org/patch/145800/ > [2] http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-cpu-wip > [3] http://patchwork.ozlabs.org/patch/145874/ >
On 03/14/2012 08:59 AM, Lai Jiangshan wrote: > On 03/13/2012 07:04 PM, Andreas Färber wrote: >> Am 13.03.2012 10:32, schrieb Lai Jiangshan: >>> On 02/16/2012 08:51 PM, Anthony Liguori wrote: >>>> On 02/16/2012 06:01 AM, Jan Kiszka wrote: >>>>> On 2012-02-16 00:16, Igor Mammedov wrote: >>>>>> +static ICCBusDeviceInfo cpu_device_info = { >>>>>> + .qdev.name = "cpu-pc", >>>>>> + .qdev.size = sizeof(CPUPC), >>>>>> + .qdev.reset = cpu_device_reset, >>>>>> + .init = cpu_device_init, >>>>>> + .qdev.props = (Property[]) { >>>>>> + DEFINE_PROP_STRING("model", CPUPC, model), >>>>> >>>>> And how do you pass in feature flags? Or the core layout? Basically both >>>>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx" >>>>> (not "pc") commands. >>>> >>>> The approach that I'd recommend is: >>>> >>>> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately. >>> >>> I just tried this for several days, it will result a huge patch, >>> it is hard for human, any suggestion? >>> (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard, >>> it still leaves huge part which needs manual conversion/fix) >>> >>> I will take part in implementing cpu-hotplug future for qemu, >>> Could you give me some tips/suggestions? What approach should I take? >> >> In short: Don't! >> >> Please instead provide feedback on my series which already does >> something like that for you, available since multiple weeks. If a >> different naming is preferred, I can change it. >> >> The approach I have taken is: >> 1. CPUState -> CPU{X86,...}State, CPUArchState as alias where necessary >> 2. Embed CPU$archState in $archCPU >> 3. Step by step move CPU_COMMON_STATE from CPUArchState into CPUState >> >> Steps 1+2 are done, 2 is partially on the list (e.g., [1]), the rest on >> my qom-cpu-wip branch [2]. >> Step 3 is shown for icount on qom-cpu-wip as well as for multiple >> target-specific fields such as target-arm features [3]. Contributions >> welcome. > > > OK, your work can't be done automatically, but it do be worth when > there are multiple target-specific fields existed. > > Anthony Liguori, Jan Kiszka, > > Could you light me what can I do for cpu-hotplug. > I saw several approaches are sent to the maillist, but they are > not accepted, so I don't know how to take part in. As I see it, there is not much to do from cpu hot-plug perspective. It's just a matter of adaptation QOM-ified cpus for usage from qdev device_add, and I'm working on it. However, there is a lot to be done in cpu unplug area: - host side: there is unaccepted patches to destroy vcpu during VM-lifecycle. They are still need to be worked on: "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" - linux guest side: kernel can receive ACPI request to unplug cpu, but does nothing with it (last time I've tested it with 3.2 kernel), You might wish to look at following mail threads: https://lkml.org/lkml/2011/9/30/18 http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html
Hi, On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote: > On 03/14/2012 08:59 AM, Lai Jiangshan wrote: > >not accepted, so I don't know how to take part in. > > As I see it, there is not much to do from cpu hot-plug perspective. > It's just a matter of adaptation QOM-ified cpus for usage from > qdev device_add, and I'm working on it. > However, there is a lot to be done in cpu unplug area: > - host side: there is unaccepted patches to destroy vcpu > during VM-lifecycle. They are still need to be worked on: > "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" > - linux guest side: kernel can receive ACPI request to unplug cpu, > but does nothing with it (last time I've tested it with 3.2 kernel), > You might wish to look at following mail threads: > https://lkml.org/lkml/2011/9/30/18 > http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html I also plan to resubmit the qemu-side of ACPI cpu unplug request: http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html so that they work independently of the "host side" patches mentioned above. It would be great for the QOMify/hotplug/icc patches to be accepted soon, since this would make unplug testing/development more straightoward. thanks, - Vasilis
On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: > Hi, > > On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote: > > On 03/14/2012 08:59 AM, Lai Jiangshan wrote: > > >not accepted, so I don't know how to take part in. > > > > As I see it, there is not much to do from cpu hot-plug perspective. > > It's just a matter of adaptation QOM-ified cpus for usage from > > qdev device_add, and I'm working on it. > > However, there is a lot to be done in cpu unplug area: > > - host side: there is unaccepted patches to destroy vcpu > > during VM-lifecycle. They are still need to be worked on: > > "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" > > - linux guest side: kernel can receive ACPI request to unplug cpu, > > but does nothing with it (last time I've tested it with 3.2 kernel), > > You might wish to look at following mail threads: > > https://lkml.org/lkml/2011/9/30/18 > > http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html > > I also plan to resubmit the qemu-side of ACPI cpu unplug request: > http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html > so that they work independently of the "host side" patches mentioned above. > > It would be great for the QOMify/hotplug/icc patches to be accepted soon, > since this would make unplug testing/development more straightoward. > On a different note, are your going to continue working on your memory hot plug series? I am going to look at it now. -- Gleb.
On 03/14/2012 10:23 AM, Gleb Natapov wrote: > On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: >> Hi, >> >> On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote: >>> On 03/14/2012 08:59 AM, Lai Jiangshan wrote: >>>> not accepted, so I don't know how to take part in. >>> >>> As I see it, there is not much to do from cpu hot-plug perspective. >>> It's just a matter of adaptation QOM-ified cpus for usage from >>> qdev device_add, and I'm working on it. >>> However, there is a lot to be done in cpu unplug area: >>> - host side: there is unaccepted patches to destroy vcpu >>> during VM-lifecycle. They are still need to be worked on: >>> "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" >>> - linux guest side: kernel can receive ACPI request to unplug cpu, >>> but does nothing with it (last time I've tested it with 3.2 kernel), >>> You might wish to look at following mail threads: >>> https://lkml.org/lkml/2011/9/30/18 >>> http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html >> >> I also plan to resubmit the qemu-side of ACPI cpu unplug request: >> http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html >> so that they work independently of the "host side" patches mentioned above. >> >> It would be great for the QOMify/hotplug/icc patches to be accepted soon, >> since this would make unplug testing/development more straightoward. >> > On a different note, are your going to continue working on your memory hot plug series? > I am going to look at it now. Memory hotplug.. that's going to be fun :-) Regards, Anthony Liguori > > -- > Gleb.
On Wed, Mar 14, 2012 at 10:32:37AM -0500, Anthony Liguori wrote: > On 03/14/2012 10:23 AM, Gleb Natapov wrote: > >On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: > >>Hi, > >> > >>On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote: > >>>On 03/14/2012 08:59 AM, Lai Jiangshan wrote: > >>>>not accepted, so I don't know how to take part in. > >>> > >>>As I see it, there is not much to do from cpu hot-plug perspective. > >>>It's just a matter of adaptation QOM-ified cpus for usage from > >>>qdev device_add, and I'm working on it. > >>>However, there is a lot to be done in cpu unplug area: > >>> - host side: there is unaccepted patches to destroy vcpu > >>> during VM-lifecycle. They are still need to be worked on: > >>> "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" > >>> - linux guest side: kernel can receive ACPI request to unplug cpu, > >>> but does nothing with it (last time I've tested it with 3.2 kernel), > >>> You might wish to look at following mail threads: > >>> https://lkml.org/lkml/2011/9/30/18 > >>> http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html > >> > >>I also plan to resubmit the qemu-side of ACPI cpu unplug request: > >>http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html > >>so that they work independently of the "host side" patches mentioned above. > >> > >>It would be great for the QOMify/hotplug/icc patches to be accepted soon, > >>since this would make unplug testing/development more straightoward. > >> > >On a different note, are your going to continue working on your memory hot plug series? > >I am going to look at it now. > > Memory hotplug.. that's going to be fun :-) > Why? What fundamental difficultly do you anticipate? -- Gleb.
On 03/14/2012 10:35 AM, Gleb Natapov wrote: > On Wed, Mar 14, 2012 at 10:32:37AM -0500, Anthony Liguori wrote: >> On 03/14/2012 10:23 AM, Gleb Natapov wrote: >>> On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: >>>> Hi, >>>> >>>> On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote: >>>>> On 03/14/2012 08:59 AM, Lai Jiangshan wrote: >>>>>> not accepted, so I don't know how to take part in. >>>>> >>>>> As I see it, there is not much to do from cpu hot-plug perspective. >>>>> It's just a matter of adaptation QOM-ified cpus for usage from >>>>> qdev device_add, and I'm working on it. >>>>> However, there is a lot to be done in cpu unplug area: >>>>> - host side: there is unaccepted patches to destroy vcpu >>>>> during VM-lifecycle. They are still need to be worked on: >>>>> "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" >>>>> - linux guest side: kernel can receive ACPI request to unplug cpu, >>>>> but does nothing with it (last time I've tested it with 3.2 kernel), >>>>> You might wish to look at following mail threads: >>>>> https://lkml.org/lkml/2011/9/30/18 >>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html >>>> >>>> I also plan to resubmit the qemu-side of ACPI cpu unplug request: >>>> http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html >>>> so that they work independently of the "host side" patches mentioned above. >>>> >>>> It would be great for the QOMify/hotplug/icc patches to be accepted soon, >>>> since this would make unplug testing/development more straightoward. >>>> >>> On a different note, are your going to continue working on your memory hot plug series? >>> I am going to look at it now. >> >> Memory hotplug.. that's going to be fun :-) >> > Why? What fundamental difficultly do you anticipate? There's still a few places in QEMU that assume that qemu_ram_get_ptr() returns a pointer that's good indefinitely. We also don't have a mechanism to revoke cpu_physical_map() pointers. Maybe the answer is reference counting and relying on being able to eventually release the memory... Of course, then an unplug followed by an immediate plug would be complicated. Hot add should be easy, hot remove will be pretty hard I think. Regards, Anthony Liguori > > -- > Gleb.
On Wed, Mar 14, 2012 at 10:42:50AM -0500, Anthony Liguori wrote: > On 03/14/2012 10:35 AM, Gleb Natapov wrote: > >On Wed, Mar 14, 2012 at 10:32:37AM -0500, Anthony Liguori wrote: > >>On 03/14/2012 10:23 AM, Gleb Natapov wrote: > >>>On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: > >>>>Hi, > >>>> > >>>>On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote: > >>>>>On 03/14/2012 08:59 AM, Lai Jiangshan wrote: > >>>>>>not accepted, so I don't know how to take part in. > >>>>> > >>>>>As I see it, there is not much to do from cpu hot-plug perspective. > >>>>>It's just a matter of adaptation QOM-ified cpus for usage from > >>>>>qdev device_add, and I'm working on it. > >>>>>However, there is a lot to be done in cpu unplug area: > >>>>> - host side: there is unaccepted patches to destroy vcpu > >>>>> during VM-lifecycle. They are still need to be worked on: > >>>>> "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm" > >>>>> - linux guest side: kernel can receive ACPI request to unplug cpu, > >>>>> but does nothing with it (last time I've tested it with 3.2 kernel), > >>>>> You might wish to look at following mail threads: > >>>>> https://lkml.org/lkml/2011/9/30/18 > >>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html > >>>> > >>>>I also plan to resubmit the qemu-side of ACPI cpu unplug request: > >>>>http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html > >>>>so that they work independently of the "host side" patches mentioned above. > >>>> > >>>>It would be great for the QOMify/hotplug/icc patches to be accepted soon, > >>>>since this would make unplug testing/development more straightoward. > >>>> > >>>On a different note, are your going to continue working on your memory hot plug series? > >>>I am going to look at it now. > >> > >>Memory hotplug.. that's going to be fun :-) > >> > >Why? What fundamental difficultly do you anticipate? > > There's still a few places in QEMU that assume that > qemu_ram_get_ptr() returns a pointer that's good indefinitely. > > We also don't have a mechanism to revoke cpu_physical_map() > pointers. Maybe the answer is reference counting and relying on > being able to eventually release the memory... Of course, then an > unplug followed by an immediate plug would be complicated. > Hmm, Avi assured me that with the memory API rework it should be easy :( grep founds nothing about qemu_ram_get_ptr() and cpu_physical_map() though. What should I look for? > Hot add should be easy, hot remove will be pretty hard I think. > > Regards, > > Anthony Liguori > > > > >-- > > Gleb. -- Gleb.
On 03/14/2012 10:54 AM, Gleb Natapov wrote: > On Wed, Mar 14, 2012 at 10:42:50AM -0500, Anthony Liguori wrote: >> >> There's still a few places in QEMU that assume that >> qemu_ram_get_ptr() returns a pointer that's good indefinitely. >> >> We also don't have a mechanism to revoke cpu_physical_map() >> pointers. Maybe the answer is reference counting and relying on >> being able to eventually release the memory... Of course, then an >> unplug followed by an immediate plug would be complicated. >> > Hmm, Avi assured me that with the memory API rework it should be easy :( > grep founds nothing about qemu_ram_get_ptr() and cpu_physical_map() > though. What should I look for? memory_region_get_ptr() and cpu_memory_physical_map(). Regards, Anthony Liguori
On Wed, Mar 14, 2012 at 10:57:15AM -0500, Anthony Liguori wrote: > On 03/14/2012 10:54 AM, Gleb Natapov wrote: > >On Wed, Mar 14, 2012 at 10:42:50AM -0500, Anthony Liguori wrote: > >> > >>There's still a few places in QEMU that assume that > >>qemu_ram_get_ptr() returns a pointer that's good indefinitely. > >> > >>We also don't have a mechanism to revoke cpu_physical_map() > >>pointers. Maybe the answer is reference counting and relying on > >>being able to eventually release the memory... Of course, then an > >>unplug followed by an immediate plug would be complicated. > >> > >Hmm, Avi assured me that with the memory API rework it should be easy :( > >grep founds nothing about qemu_ram_get_ptr() and cpu_physical_map() > >though. What should I look for? > > memory_region_get_ptr() and cpu_memory_physical_map(). > cpu_physical_memory_map() and qemu_get_ram_ptr() (called only via memory_region_get_ram_ptr() by device model). According to qemu_get_ram_ptr() comment it should be used only to access device memory, not RAM (it is also used in softmmu code, but lets leave this for now :)). cpu_physical_memory_map() is used for DMA. When guest confirms memory removal by calling _EJ() ACPI methods no DMA should be directed to that memory slot. We can refcount slot during map/unmap and do not remove a slot it refcount > 0, or we can kill guest if this happens. That what real HW would do. -- Gleb.
On Wed, Mar 14, 2012 at 05:23:24PM +0200, Gleb Natapov wrote: > On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: > > Hi, > > > > > On a different note, are your going to continue working on your memory hot plug series? > I am going to look at it now. The memory hotplug patch from September has several issues (e.g. unplug/memory free isn't really performed at the qemu level) and is outdated (qemu-kvm-0.15 and seabios-0.6.1 timeframe). I 've continued working on it against master/new memory API and I will resubmit for comments. thanks, - Vasilis
On Wed, Mar 14, 2012 at 08:55:08PM +0100, Vasilis Liaskovitis wrote: > On Wed, Mar 14, 2012 at 05:23:24PM +0200, Gleb Natapov wrote: > > On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote: > > > Hi, > > > > > > > > On a different note, are your going to continue working on your memory hot plug series? > > I am going to look at it now. > > The memory hotplug patch from September has several issues (e.g. unplug/memory > free isn't really performed at the qemu level) and is outdated (qemu-kvm-0.15 > and seabios-0.6.1 timeframe). > I 've continued working on it against master/new memory API and I will resubmit > for comments. > Thanks, I've commented on your previous submission. When do you expect to send update patches? -- Gleb.
diff --git a/hw/pc.c b/hw/pc.c index 33d8090..b8db5dc 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) } } +typedef struct CPUPC { + ICCBusDevice busdev; + char *model; + CPUState state; +} CPUPC; + static void pc_cpu_reset(void *opaque) { CPUState *env = opaque; @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque) env->halted = !cpu_is_bsp(env); } -static CPUState *pc_new_cpu(const char *cpu_model) +static DeviceState *pc_new_cpu(const char *cpu_model) { - CPUState *env; + DeviceState *dev; + BusState *b; - env = cpu_init(cpu_model); - if (!env) { - fprintf(stderr, "Unable to find x86 CPU definition\n"); - exit(1); + b = get_icc_bus(); + dev = qdev_create(b, "cpu-pc"); + if (!dev) { + return NULL; + } + qdev_prop_set_string(dev, "model", g_strdup(cpu_model)); + if (qdev_init(dev) < 0) { + return NULL; + } + return dev; +} + +static int cpu_device_init(ICCBusDevice *dev) +{ + CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev); + CPUState *env = &cpu->state; + + if (cpu_x86_init_inplace(env, cpu->model) < 0) { + return -1; } + if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { env->apic_state = apic_init(env, env->cpuid_apic_id); } - qemu_register_reset(pc_cpu_reset, env); + + return 0; +} + +static void cpu_device_reset(DeviceState *dev) { + CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev); + CPUState *env = &cpu->state; pc_cpu_reset(env); - return env; } +static ICCBusDeviceInfo cpu_device_info = { + .qdev.name = "cpu-pc", + .qdev.size = sizeof(CPUPC), + .qdev.reset = cpu_device_reset, + .init = cpu_device_init, + .qdev.props = (Property[]) { + DEFINE_PROP_STRING("model", CPUPC, model), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static void pc_register_devices(void) +{ + iccbus_register_devinfo(&cpu_device_info); +} + +device_init(pc_register_devices); + void pc_cpus_init(const char *cpu_model) { int i; diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 7e66bcf..830b65e 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -775,6 +775,7 @@ typedef struct CPUX86State { } CPUX86State; CPUX86State *cpu_x86_init(const char *cpu_model); +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model); int cpu_x86_exec(CPUX86State *s); void cpu_x86_close(CPUX86State *s); void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg); diff --git a/target-i386/helper.c b/target-i386/helper.c index 2586aff..df2f5ba 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, return 1; } -CPUX86State *cpu_x86_init(const char *cpu_model) +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model) { - CPUX86State *env; static int inited; - env = g_malloc0(sizeof(CPUX86State)); + if (cpu_x86_register(env, cpu_model) < 0) { + fprintf(stderr, "Unable to find x86 CPU definition\n"); + return -1; + } cpu_exec_init(env); env->cpu_model_str = cpu_model; @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model) cpu_set_debug_excp_handler(breakpoint_handler); #endif } - if (cpu_x86_register(env, cpu_model) < 0) { - cpu_x86_close(env); - return NULL; - } env->cpuid_apic_id = env->cpu_index; mce_init(env); qemu_init_vcpu(env); - return env; + return 0; } +CPUX86State *cpu_x86_init(const char *cpu_model) +{ + CPUX86State *env; + + env = g_malloc0(sizeof(CPUX86State)); + if (cpu_x86_init_inplace(env, cpu_model) < 0) { + g_free(env); + return NULL; + } + + return env; +} #if !defined(CONFIG_USER_ONLY) void do_cpu_init(CPUState *env) {
Convert pc cpu to qdev device that is attached to icc bus, later hot-plug ability of icc bus will allow to implement cpu hot-plug. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/pc.c | 62 +++++++++++++++++++++++++++++++++++++++++++------ target-i386/cpu.h | 1 + target-i386/helper.c | 26 ++++++++++++++------ 3 files changed, 73 insertions(+), 16 deletions(-)