* [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:03 ` David Gibson
` (2 more replies)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 02/11] exec: Remove cpu from cpus list during cpu_exec_exit() Bharata B Rao
` (9 subsequent siblings)
10 siblings, 3 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
Prevent guests from booting with CPU topologies that have partially
filled CPU cores or can result in partially filled CPU cores after
CPU hotplug like
-smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
-smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
This is enforced by introducing MachineClass::validate_smp_config()
that gets called from generic SMP parsing code. Machine type versions
that don't want to enforce this can override this method.
TODO: Only sPAPR and PC changes are done in this patch, other archs
will be touched after there is agreement on this approach.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/core/machine.c | 20 ++++++++++++++++++++
hw/i386/pc_piix.c | 7 +++++++
hw/i386/pc_q35.c | 7 +++++++
hw/ppc/spapr.c | 7 +++++++
include/hw/boards.h | 1 +
vl.c | 4 ++++
6 files changed, 46 insertions(+)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c46ddc7..b66c101 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -336,6 +336,25 @@ static void machine_init_notify(Notifier *notifier, void *data)
foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
}
+static int validate_smp_config_generic(int smp_cpus, int max_cpus,
+ int smp_threads)
+{
+ if (smp_cpus % smp_threads) {
+ error_report("cpu topology: "
+ "smp_cpus (%u) should be multiple of threads (%u) ",
+ smp_cpus, smp_threads);
+ return 1;
+ }
+
+ if (max_cpus % smp_threads) {
+ error_report("cpu topology: "
+ "max_cpus (%u) should be multiple of threads (%u) ",
+ max_cpus, smp_threads);
+ return 1;
+ }
+ return 0;
+}
+
static void machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -343,6 +362,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
/* Default 128 MB as guest ram size */
mc->default_ram_size = 128 * M_BYTE;
mc->rom_file_has_mr = true;
+ mc->validate_smp_config = validate_smp_config_generic;
}
static void machine_class_base_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 438cdae..dd4bba1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -397,6 +397,12 @@ static void pc_xen_hvm_init(MachineState *machine)
}
#endif
+static int pc_validate_smp_config_default(int smp_cpus, int max_cpus,
+ int smp_threads)
+{
+ return 0;
+}
+
#define DEFINE_I440FX_MACHINE(suffix, name, compatfn, optionfn) \
static void pc_init_##suffix(MachineState *machine) \
{ \
@@ -434,6 +440,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
pc_i440fx_2_6_machine_options(m);
m->alias = NULL;
m->is_default = 0;
+ m->validate_smp_config = pc_validate_smp_config_default;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 412b3cd..80ce9e8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -325,6 +325,12 @@ static void pc_compat_1_4(MachineState *machine)
pc_compat_1_5(machine);
}
+static int pc_q35_validate_smp_config_default(int smp_cpus, int max_cpus,
+ int smp_threads)
+{
+ return 0;
+}
+
#define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
static void pc_init_##suffix(MachineState *machine) \
{ \
@@ -362,6 +368,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
{
pc_q35_2_6_machine_options(m);
m->alias = NULL;
+ m->validate_smp_config = pc_q35_validate_smp_config_default;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 414e0f9b..a330169 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2320,6 +2320,12 @@ static const TypeInfo spapr_machine_info = {
},
};
+static int spapr_validate_smp_config_default(int smp_cpus, int max_cpus,
+ int smp_threads)
+{
+ return 0;
+}
+
#define DEFINE_SPAPR_MACHINE(suffix, verstr, latest) \
static void spapr_machine_##suffix##_class_init(ObjectClass *oc, \
void *data) \
@@ -2379,6 +2385,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
spapr_machine_2_6_class_options(mc);
smc->use_ohci_by_default = true;
SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
+ mc->validate_smp_config = spapr_validate_smp_config_default;
}
DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..39091bf 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -99,6 +99,7 @@ struct MachineClass {
HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
DeviceState *dev);
unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
+ int (*validate_smp_config)(int smp_cpus, int max_cpus, int smp_threads);
};
/**
diff --git a/vl.c b/vl.c
index 5aaea77..4b36a49 100644
--- a/vl.c
+++ b/vl.c
@@ -4132,6 +4132,10 @@ int main(int argc, char **argv, char **envp)
smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
+ if (machine_class->validate_smp_config(smp_cpus, max_cpus, smp_threads)) {
+ exit(1);
+ }
+
machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
if (max_cpus > machine_class->max_cpus) {
error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores Bharata B Rao
@ 2016-01-12 4:03 ` David Gibson
2016-01-12 23:24 ` Alexey Kardashevskiy
2016-01-23 13:47 ` Eduardo Habkost
2 siblings, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:03 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 6657 bytes --]
On Fri, Jan 08, 2016 at 12:25:09PM +0530, Bharata B Rao wrote:
> Prevent guests from booting with CPU topologies that have partially
> filled CPU cores or can result in partially filled CPU cores after
> CPU hotplug like
>
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
>
> This is enforced by introducing MachineClass::validate_smp_config()
> that gets called from generic SMP parsing code. Machine type versions
> that don't want to enforce this can override this method.
>
> TODO: Only sPAPR and PC changes are done in this patch, other archs
> will be touched after there is agreement on this approach.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Personally, I think this restriciton should be enforced always, and
for non-full sockets as well. But I'm happy enough with this if it
lets us move forwards.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/core/machine.c | 20 ++++++++++++++++++++
> hw/i386/pc_piix.c | 7 +++++++
> hw/i386/pc_q35.c | 7 +++++++
> hw/ppc/spapr.c | 7 +++++++
> include/hw/boards.h | 1 +
> vl.c | 4 ++++
> 6 files changed, 46 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c46ddc7..b66c101 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -336,6 +336,25 @@ static void machine_init_notify(Notifier *notifier, void *data)
> foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> }
>
> +static int validate_smp_config_generic(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + if (smp_cpus % smp_threads) {
> + error_report("cpu topology: "
> + "smp_cpus (%u) should be multiple of threads (%u) ",
> + smp_cpus, smp_threads);
> + return 1;
> + }
> +
> + if (max_cpus % smp_threads) {
> + error_report("cpu topology: "
> + "max_cpus (%u) should be multiple of threads (%u) ",
> + max_cpus, smp_threads);
> + return 1;
> + }
> + return 0;
> +}
> +
> static void machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -343,6 +362,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
> /* Default 128 MB as guest ram size */
> mc->default_ram_size = 128 * M_BYTE;
> mc->rom_file_has_mr = true;
> + mc->validate_smp_config = validate_smp_config_generic;
> }
>
> static void machine_class_base_init(ObjectClass *oc, void *data)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 438cdae..dd4bba1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -397,6 +397,12 @@ static void pc_xen_hvm_init(MachineState *machine)
> }
> #endif
>
> +static int pc_validate_smp_config_default(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + return 0;
> +}
> +
> #define DEFINE_I440FX_MACHINE(suffix, name, compatfn, optionfn) \
> static void pc_init_##suffix(MachineState *machine) \
> { \
> @@ -434,6 +440,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
> pc_i440fx_2_6_machine_options(m);
> m->alias = NULL;
> m->is_default = 0;
> + m->validate_smp_config = pc_validate_smp_config_default;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
> }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 412b3cd..80ce9e8 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -325,6 +325,12 @@ static void pc_compat_1_4(MachineState *machine)
> pc_compat_1_5(machine);
> }
>
> +static int pc_q35_validate_smp_config_default(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + return 0;
> +}
> +
> #define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
> static void pc_init_##suffix(MachineState *machine) \
> { \
> @@ -362,6 +368,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
> {
> pc_q35_2_6_machine_options(m);
> m->alias = NULL;
> + m->validate_smp_config = pc_q35_validate_smp_config_default;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
> }
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 414e0f9b..a330169 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2320,6 +2320,12 @@ static const TypeInfo spapr_machine_info = {
> },
> };
>
> +static int spapr_validate_smp_config_default(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + return 0;
> +}
> +
> #define DEFINE_SPAPR_MACHINE(suffix, verstr, latest) \
> static void spapr_machine_##suffix##_class_init(ObjectClass *oc, \
> void *data) \
> @@ -2379,6 +2385,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> spapr_machine_2_6_class_options(mc);
> smc->use_ohci_by_default = true;
> SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> + mc->validate_smp_config = spapr_validate_smp_config_default;
> }
>
> DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..39091bf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -99,6 +99,7 @@ struct MachineClass {
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> + int (*validate_smp_config)(int smp_cpus, int max_cpus, int smp_threads);
> };
>
> /**
> diff --git a/vl.c b/vl.c
> index 5aaea77..4b36a49 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4132,6 +4132,10 @@ int main(int argc, char **argv, char **envp)
>
> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>
> + if (machine_class->validate_smp_config(smp_cpus, max_cpus, smp_threads)) {
> + exit(1);
> + }
> +
> machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
> if (max_cpus > machine_class->max_cpus) {
> error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores Bharata B Rao
2016-01-12 4:03 ` David Gibson
@ 2016-01-12 23:24 ` Alexey Kardashevskiy
2016-01-23 13:47 ` Eduardo Habkost
2 siblings, 0 replies; 39+ messages in thread
From: Alexey Kardashevskiy @ 2016-01-12 23:24 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel
Cc: ehabkost, mdroth, agraf, pbonzini, qemu-ppc, tyreld, nfont,
imammedo, afaerber, david
On 01/08/2016 05:55 PM, Bharata B Rao wrote:
> Prevent guests from booting with CPU topologies that have partially
> filled CPU cores or can result in partially filled CPU cores after
> CPU hotplug like
>
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
>
> This is enforced by introducing MachineClass::validate_smp_config()
> that gets called from generic SMP parsing code. Machine type versions
> that don't want to enforce this can override this method.
>
> TODO: Only sPAPR and PC changes are done in this patch, other archs
> will be touched after there is agreement on this approach.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/core/machine.c | 20 ++++++++++++++++++++
> hw/i386/pc_piix.c | 7 +++++++
> hw/i386/pc_q35.c | 7 +++++++
> hw/ppc/spapr.c | 7 +++++++
> include/hw/boards.h | 1 +
> vl.c | 4 ++++
> 6 files changed, 46 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c46ddc7..b66c101 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -336,6 +336,25 @@ static void machine_init_notify(Notifier *notifier, void *data)
> foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> }
>
> +static int validate_smp_config_generic(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + if (smp_cpus % smp_threads) {
> + error_report("cpu topology: "
> + "smp_cpus (%u) should be multiple of threads (%u) ",
> + smp_cpus, smp_threads);
> + return 1;
> + }
> +
> + if (max_cpus % smp_threads) {
> + error_report("cpu topology: "
> + "max_cpus (%u) should be multiple of threads (%u) ",
> + max_cpus, smp_threads);
> + return 1;
> + }
> + return 0;
> +}
If you decide to repost - here is incorrect indent.
> +
> static void machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -343,6 +362,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
> /* Default 128 MB as guest ram size */
> mc->default_ram_size = 128 * M_BYTE;
> mc->rom_file_has_mr = true;
> + mc->validate_smp_config = validate_smp_config_generic;
> }
>
> static void machine_class_base_init(ObjectClass *oc, void *data)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 438cdae..dd4bba1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -397,6 +397,12 @@ static void pc_xen_hvm_init(MachineState *machine)
> }
> #endif
>
> +static int pc_validate_smp_config_default(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + return 0;
> +}
> +
> #define DEFINE_I440FX_MACHINE(suffix, name, compatfn, optionfn) \
> static void pc_init_##suffix(MachineState *machine) \
> { \
> @@ -434,6 +440,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
> pc_i440fx_2_6_machine_options(m);
> m->alias = NULL;
> m->is_default = 0;
> + m->validate_smp_config = pc_validate_smp_config_default;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
> }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 412b3cd..80ce9e8 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -325,6 +325,12 @@ static void pc_compat_1_4(MachineState *machine)
> pc_compat_1_5(machine);
> }
>
> +static int pc_q35_validate_smp_config_default(int smp_cpus, int max_cpus,
> + int smp_threads)
> +{
> + return 0;
> +}
> +
> #define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
> static void pc_init_##suffix(MachineState *machine) \
> { \
> @@ -362,6 +368,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
> {
> pc_q35_2_6_machine_options(m);
> m->alias = NULL;
> + m->validate_smp_config = pc_q35_validate_smp_config_default;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
> }
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 414e0f9b..a330169 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2320,6 +2320,12 @@ static const TypeInfo spapr_machine_info = {
> },
> };
>
> +static int spapr_validate_smp_config_default(int smp_cpus, int max_cpus,
> + int smp_threads)
and here.
> +{
> + return 0;
> +}
> +
> #define DEFINE_SPAPR_MACHINE(suffix, verstr, latest) \
> static void spapr_machine_##suffix##_class_init(ObjectClass *oc, \
> void *data) \
> @@ -2379,6 +2385,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> spapr_machine_2_6_class_options(mc);
> smc->use_ohci_by_default = true;
> SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> + mc->validate_smp_config = spapr_validate_smp_config_default;
> }
>
> DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..39091bf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -99,6 +99,7 @@ struct MachineClass {
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> + int (*validate_smp_config)(int smp_cpus, int max_cpus, int smp_threads);
> };
>
> /**
> diff --git a/vl.c b/vl.c
> index 5aaea77..4b36a49 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4132,6 +4132,10 @@ int main(int argc, char **argv, char **envp)
>
> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>
> + if (machine_class->validate_smp_config(smp_cpus, max_cpus, smp_threads)) {
> + exit(1);
> + }
> +
> machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
> if (max_cpus > machine_class->max_cpus) {
> error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
>
--
Alexey
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores Bharata B Rao
2016-01-12 4:03 ` David Gibson
2016-01-12 23:24 ` Alexey Kardashevskiy
@ 2016-01-23 13:47 ` Eduardo Habkost
2016-01-25 8:57 ` Bharata B Rao
2 siblings, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2016-01-23 13:47 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, david
On Fri, Jan 08, 2016 at 12:25:09PM +0530, Bharata B Rao wrote:
> Prevent guests from booting with CPU topologies that have partially
> filled CPU cores or can result in partially filled CPU cores after
> CPU hotplug like
>
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
>
> This is enforced by introducing MachineClass::validate_smp_config()
> that gets called from generic SMP parsing code. Machine type versions
> that don't want to enforce this can override this method.
>
> TODO: Only sPAPR and PC changes are done in this patch, other archs
> will be touched after there is agreement on this approach.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/core/machine.c | 20 ++++++++++++++++++++
> hw/i386/pc_piix.c | 7 +++++++
> hw/i386/pc_q35.c | 7 +++++++
> hw/ppc/spapr.c | 7 +++++++
> include/hw/boards.h | 1 +
> vl.c | 4 ++++
> 6 files changed, 46 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c46ddc7..b66c101 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -336,6 +336,25 @@ static void machine_init_notify(Notifier *notifier, void *data)
> foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> }
>
> +static int validate_smp_config_generic(int smp_cpus, int max_cpus,
> + int smp_threads)
Please make it use a Error** parameter to return error
information, and let the caller decide what to do with the error
message. One day the mc->validate_smp_config() call may be moved
inside a function that returns error information using Error**
and needs to propagate it to the caller.
> +{
> + if (smp_cpus % smp_threads) {
> + error_report("cpu topology: "
> + "smp_cpus (%u) should be multiple of threads (%u) ",
> + smp_cpus, smp_threads);
> + return 1;
> + }
> +
> + if (max_cpus % smp_threads) {
> + error_report("cpu topology: "
> + "max_cpus (%u) should be multiple of threads (%u) ",
> + max_cpus, smp_threads);
> + return 1;
> + }
> + return 0;
> +}
> +
[...]
> diff --git a/vl.c b/vl.c
> index 5aaea77..4b36a49 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4132,6 +4132,10 @@ int main(int argc, char **argv, char **envp)
>
> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>
> + if (machine_class->validate_smp_config(smp_cpus, max_cpus, smp_threads)) {
> + exit(1);
> + }
> +
> machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
> if (max_cpus > machine_class->max_cpus) {
> error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> --
> 2.1.0
>
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores
2016-01-23 13:47 ` Eduardo Habkost
@ 2016-01-25 8:57 ` Bharata B Rao
2016-01-26 17:47 ` Eduardo Habkost
0 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-25 8:57 UTC (permalink / raw)
To: Eduardo Habkost
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, david
On Sat, Jan 23, 2016 at 11:47:22AM -0200, Eduardo Habkost wrote:
> On Fri, Jan 08, 2016 at 12:25:09PM +0530, Bharata B Rao wrote:
> > Prevent guests from booting with CPU topologies that have partially
> > filled CPU cores or can result in partially filled CPU cores after
> > CPU hotplug like
> >
> > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> >
> > This is enforced by introducing MachineClass::validate_smp_config()
> > that gets called from generic SMP parsing code. Machine type versions
> > that don't want to enforce this can override this method.
> >
> > TODO: Only sPAPR and PC changes are done in this patch, other archs
> > will be touched after there is agreement on this approach.
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > hw/core/machine.c | 20 ++++++++++++++++++++
> > hw/i386/pc_piix.c | 7 +++++++
> > hw/i386/pc_q35.c | 7 +++++++
> > hw/ppc/spapr.c | 7 +++++++
> > include/hw/boards.h | 1 +
> > vl.c | 4 ++++
> > 6 files changed, 46 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index c46ddc7..b66c101 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -336,6 +336,25 @@ static void machine_init_notify(Notifier *notifier, void *data)
> > foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> > }
> >
> > +static int validate_smp_config_generic(int smp_cpus, int max_cpus,
> > + int smp_threads)
>
> Please make it use a Error** parameter to return error
> information, and let the caller decide what to do with the error
> message. One day the mc->validate_smp_config() call may be moved
> inside a function that returns error information using Error**
> and needs to propagate it to the caller.
Sure, will make this change.
I see that not all target archs use machine type versions and even
if they do it will not be a one-line changer (as shown for i386 and sPAPR in
this patch) for them to override mc->validate_smp_config() for older
versions.
So am I expected to change .class_init routine for all these archs by
setting mc->validate_smp_config() to a NOP validate_smp_config()
so that the default validate_smp_config() defined for TYPE_MACHINE
is overridden ? In order to preserve the existing behaviour with
CPU topology for all args, it appears that such a change would be
necessary, but I am not sure how to test all of them. Will just a compile
test do ?
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores
2016-01-25 8:57 ` Bharata B Rao
@ 2016-01-26 17:47 ` Eduardo Habkost
0 siblings, 0 replies; 39+ messages in thread
From: Eduardo Habkost @ 2016-01-26 17:47 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, david
On Mon, Jan 25, 2016 at 02:27:25PM +0530, Bharata B Rao wrote:
> On Sat, Jan 23, 2016 at 11:47:22AM -0200, Eduardo Habkost wrote:
> > On Fri, Jan 08, 2016 at 12:25:09PM +0530, Bharata B Rao wrote:
> > > Prevent guests from booting with CPU topologies that have partially
> > > filled CPU cores or can result in partially filled CPU cores after
> > > CPU hotplug like
> > >
> > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > >
> > > This is enforced by introducing MachineClass::validate_smp_config()
> > > that gets called from generic SMP parsing code. Machine type versions
> > > that don't want to enforce this can override this method.
> > >
> > > TODO: Only sPAPR and PC changes are done in this patch, other archs
> > > will be touched after there is agreement on this approach.
> > >
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > > hw/core/machine.c | 20 ++++++++++++++++++++
> > > hw/i386/pc_piix.c | 7 +++++++
> > > hw/i386/pc_q35.c | 7 +++++++
> > > hw/ppc/spapr.c | 7 +++++++
> > > include/hw/boards.h | 1 +
> > > vl.c | 4 ++++
> > > 6 files changed, 46 insertions(+)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index c46ddc7..b66c101 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -336,6 +336,25 @@ static void machine_init_notify(Notifier *notifier, void *data)
> > > foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> > > }
> > >
> > > +static int validate_smp_config_generic(int smp_cpus, int max_cpus,
> > > + int smp_threads)
> >
> > Please make it use a Error** parameter to return error
> > information, and let the caller decide what to do with the error
> > message. One day the mc->validate_smp_config() call may be moved
> > inside a function that returns error information using Error**
> > and needs to propagate it to the caller.
>
> Sure, will make this change.
>
> I see that not all target archs use machine type versions and even
> if they do it will not be a one-line changer (as shown for i386 and sPAPR in
> this patch) for them to override mc->validate_smp_config() for older
> versions.
>
> So am I expected to change .class_init routine for all these archs by
> setting mc->validate_smp_config() to a NOP validate_smp_config()
> so that the default validate_smp_config() defined for TYPE_MACHINE
> is overridden ? In order to preserve the existing behaviour with
> CPU topology for all args, it appears that such a change would be
> necessary, but I am not sure how to test all of them. Will just a compile
> test do ?
I am not sure I understand the question: if you want most machine
classes to have a NOP validate_smp_config(), why do you want to
make validate_smp_config_generic() the default in TYPE_MACHINE?
Just make a more convenient default in TYPE_MACHINE, and override
it in the pc and spapr base classes.
In other words, if you don't want to risk breaking other machine
classes, you just need explicit one-line validate_smp_config
assignments in 5 class_init/machine_options functions:
Class | validate_smp_config pointer
-------------------+------------------------------
TYPE_MACHINE | NULL (no explicit assignment)
TYPE_PC_MACHINE | validate_smp_config_generic
TYPE_SPAPR_MACHINE | validate_smp_config_generic
pc-i440fx-2.5 | NULL
pc-q35-2.5 | NULL
spapr-2.5 | NULL
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 02/11] exec: Remove cpu from cpus list during cpu_exec_exit()
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:06 ` David Gibson
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 03/11] exec: Do vmstate unregistration from cpu_exec_exit() Bharata B Rao
` (8 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
CPUState *cpu gets added to the cpus list during cpu_exec_init(). It
should be removed from cpu_exec_exit().
cpu_exec_init() is called from generic CPU::instance_finalize and some
archs like PowerPC call it from CPU unrealizefn. So ensure that we
dequeue the cpu only once.
Now -1 value for cpu->cpu_index indicates that we have already dequeued
the cpu for CONFIG_USER_ONLY case also.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
exec.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/exec.c b/exec.c
index 8718a75..25c0f36 100644
--- a/exec.c
+++ b/exec.c
@@ -578,6 +578,7 @@ void cpu_exec_exit(CPUState *cpu)
return;
}
+ QTAILQ_REMOVE(&cpus, cpu, node);
bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
cpu->cpu_index = -1;
}
@@ -596,6 +597,15 @@ static int cpu_get_free_index(Error **errp)
void cpu_exec_exit(CPUState *cpu)
{
+ cpu_list_lock();
+ if (cpu->cpu_index == -1) {
+ cpu_list_unlock();
+ return;
+ }
+
+ QTAILQ_REMOVE(&cpus, cpu, node);
+ cpu->cpu_index = -1;
+ cpu_list_unlock();
}
#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/11] exec: Remove cpu from cpus list during cpu_exec_exit()
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 02/11] exec: Remove cpu from cpus list during cpu_exec_exit() Bharata B Rao
@ 2016-01-12 4:06 ` David Gibson
0 siblings, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:06 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]
On Fri, Jan 08, 2016 at 12:25:10PM +0530, Bharata B Rao wrote:
> CPUState *cpu gets added to the cpus list during cpu_exec_init(). It
> should be removed from cpu_exec_exit().
>
> cpu_exec_init() is called from generic CPU::instance_finalize and some
> archs like PowerPC call it from CPU unrealizefn. So ensure that we
> dequeue the cpu only once.
>
> Now -1 value for cpu->cpu_index indicates that we have already dequeued
> the cpu for CONFIG_USER_ONLY case also.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> exec.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index 8718a75..25c0f36 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -578,6 +578,7 @@ void cpu_exec_exit(CPUState *cpu)
> return;
> }
>
> + QTAILQ_REMOVE(&cpus, cpu, node);
> bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> cpu->cpu_index = -1;
> }
> @@ -596,6 +597,15 @@ static int cpu_get_free_index(Error **errp)
>
> void cpu_exec_exit(CPUState *cpu)
> {
> + cpu_list_lock();
> + if (cpu->cpu_index == -1) {
> + cpu_list_unlock();
> + return;
> + }
> +
> + QTAILQ_REMOVE(&cpus, cpu, node);
> + cpu->cpu_index = -1;
> + cpu_list_unlock();
> }
> #endif
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 03/11] exec: Do vmstate unregistration from cpu_exec_exit()
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores Bharata B Rao
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 02/11] exec: Remove cpu from cpus list during cpu_exec_exit() Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init() Bharata B Rao
` (7 subsequent siblings)
10 siblings, 0 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
cpu_exec_init() does vmstate_register and register_savevm for the CPU device.
These need to be undone from cpu_exec_exit(). These changes are needed to
support CPU hot removal and also to correctly fail hotplug attempts
beyond max_cpus.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
exec.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/exec.c b/exec.c
index 25c0f36..d56cf71 100644
--- a/exec.c
+++ b/exec.c
@@ -573,6 +573,8 @@ static int cpu_get_free_index(Error **errp)
void cpu_exec_exit(CPUState *cpu)
{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+
if (cpu->cpu_index == -1) {
/* cpu_index was never allocated by this @cpu or was already freed. */
return;
@@ -581,6 +583,15 @@ void cpu_exec_exit(CPUState *cpu)
QTAILQ_REMOVE(&cpus, cpu, node);
bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
cpu->cpu_index = -1;
+ if (cc->vmsd != NULL) {
+ vmstate_unregister(NULL, cc->vmsd, cpu);
+ }
+#if defined(CPU_SAVE_VERSION)
+ unregister_savevm(NULL, "cpu", cpu->env_ptr);
+#endif
+ if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+ vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+ }
}
#else
@@ -597,6 +608,8 @@ static int cpu_get_free_index(Error **errp)
void cpu_exec_exit(CPUState *cpu)
{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+
cpu_list_lock();
if (cpu->cpu_index == -1) {
cpu_list_unlock();
@@ -606,6 +619,13 @@ void cpu_exec_exit(CPUState *cpu)
QTAILQ_REMOVE(&cpus, cpu, node);
cpu->cpu_index = -1;
cpu_list_unlock();
+
+ if (cc->vmsd != NULL) {
+ vmstate_unregister(NULL, cc->vmsd, cpu);
+ }
+ if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+ vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+ }
}
#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init()
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (2 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 03/11] exec: Do vmstate unregistration from cpu_exec_exit() Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:09 ` David Gibson
2016-01-23 13:31 ` Eduardo Habkost
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects Bharata B Rao
` (6 subsequent siblings)
10 siblings, 2 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
Don't do CPU realization from cpu_generic_init(). With this
cpu_generic_init() will be used to just create CPU threads and they
should be realized separately from realizefn call.
Convert the existing callers to do explicit realization.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
qom/cpu.c | 6 ------
target-arm/helper.c | 16 +++++++++++++++-
target-cris/cpu.c | 16 +++++++++++++++-
target-lm32/helper.c | 16 +++++++++++++++-
target-moxie/cpu.c | 16 +++++++++++++++-
target-openrisc/cpu.c | 16 +++++++++++++++-
target-ppc/translate_init.c | 16 +++++++++++++++-
target-sh4/cpu.c | 16 +++++++++++++++-
target-tricore/helper.c | 16 +++++++++++++++-
target-unicore32/helper.c | 16 +++++++++++++++-
10 files changed, 135 insertions(+), 15 deletions(-)
diff --git a/qom/cpu.c b/qom/cpu.c
index fb80d13a..e7a17c1 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -63,13 +63,7 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
featurestr = strtok(NULL, ",");
cc->parse_features(cpu, featurestr, &err);
g_free(str);
- if (err != NULL) {
- goto out;
- }
-
- object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-out:
if (err != NULL) {
error_report_err(err);
object_unref(OBJECT(cpu));
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 59d5a41..cdbb743 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4546,7 +4546,21 @@ void register_cp_regs_for_features(ARMCPU *cpu)
ARMCPU *cpu_arm_init(const char *cpu_model)
{
- return ARM_CPU(cpu_generic_init(TYPE_ARM_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_ARM_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return ARM_CPU(cpu);
+ }
}
void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 8eaf5a5..d2c0822 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -89,7 +89,21 @@ static ObjectClass *cris_cpu_class_by_name(const char *cpu_model)
CRISCPU *cpu_cris_init(const char *cpu_model)
{
- return CRIS_CPU(cpu_generic_init(TYPE_CRIS_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_CRIS_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return CRIS_CPU(cpu);
+ }
}
/* Sort alphabetically by VR. */
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index e26c133..49ac960 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -218,7 +218,21 @@ bool lm32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
LM32CPU *cpu_lm32_init(const char *cpu_model)
{
- return LM32_CPU(cpu_generic_init(TYPE_LM32_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_LM32_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return LM32_CPU(cpu);
+ }
}
/* Some soc ignores the MSB on the address bus. Thus creating a shadow memory
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 0c60c65..5989fa6 100644
--- a/target-moxie/cpu.c
+++ b/target-moxie/cpu.c
@@ -152,7 +152,21 @@ static const MoxieCPUInfo moxie_cpus[] = {
MoxieCPU *cpu_moxie_init(const char *cpu_model)
{
- return MOXIE_CPU(cpu_generic_init(TYPE_MOXIE_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_MOXIE_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return MOXIE_CPU(cpu);
+ }
}
static void cpu_register(const MoxieCPUInfo *info)
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index cc5e2d1..873eafb 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -222,7 +222,21 @@ static void openrisc_cpu_register_types(void)
OpenRISCCPU *cpu_openrisc_init(const char *cpu_model)
{
- return OPENRISC_CPU(cpu_generic_init(TYPE_OPENRISC_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_OPENRISC_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return OPENRISC_CPU(cpu);
+ }
}
/* Sort alphabetically by type name, except for "any". */
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index e88dc7f..d5ae53e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9373,7 +9373,21 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
PowerPCCPU *cpu_ppc_init(const char *cpu_model)
{
- return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_POWERPC_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return POWERPC_CPU(cpu);
+ }
}
/* Sort by PVR, ordering special case "host" last. */
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index d7e2fbd..e5151a0 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -155,7 +155,21 @@ static ObjectClass *superh_cpu_class_by_name(const char *cpu_model)
SuperHCPU *cpu_sh4_init(const char *cpu_model)
{
- return SUPERH_CPU(cpu_generic_init(TYPE_SUPERH_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_SUPERH_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return SUPERH_CPU(cpu);
+ }
}
static void sh7750r_cpu_initfn(Object *obj)
diff --git a/target-tricore/helper.c b/target-tricore/helper.c
index 1b70429..bf164fc 100644
--- a/target-tricore/helper.c
+++ b/target-tricore/helper.c
@@ -83,7 +83,21 @@ int cpu_tricore_handle_mmu_fault(CPUState *cs, target_ulong address,
TriCoreCPU *cpu_tricore_init(const char *cpu_model)
{
- return TRICORE_CPU(cpu_generic_init(TYPE_TRICORE_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_TRICORE_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return TRICORE_CPU(cpu);
+ }
}
static void tricore_cpu_list_entry(gpointer data, gpointer user_data)
diff --git a/target-unicore32/helper.c b/target-unicore32/helper.c
index ae63277..e47bb12 100644
--- a/target-unicore32/helper.c
+++ b/target-unicore32/helper.c
@@ -27,7 +27,21 @@
UniCore32CPU *uc32_cpu_init(const char *cpu_model)
{
- return UNICORE32_CPU(cpu_generic_init(TYPE_UNICORE32_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_UNICORE32_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return UNICORE32_CPU(cpu);
+ }
}
uint32_t HELPER(clo)(uint32_t x)
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init()
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init() Bharata B Rao
@ 2016-01-12 4:09 ` David Gibson
2016-01-23 13:31 ` Eduardo Habkost
1 sibling, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:09 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 634 bytes --]
On Fri, Jan 08, 2016 at 12:25:12PM +0530, Bharata B Rao wrote:
> Don't do CPU realization from cpu_generic_init(). With this
> cpu_generic_init() will be used to just create CPU threads and they
> should be realized separately from realizefn call.
>
> Convert the existing callers to do explicit realization.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init()
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init() Bharata B Rao
2016-01-12 4:09 ` David Gibson
@ 2016-01-23 13:31 ` Eduardo Habkost
1 sibling, 0 replies; 39+ messages in thread
From: Eduardo Habkost @ 2016-01-23 13:31 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, david
On Fri, Jan 08, 2016 at 12:25:12PM +0530, Bharata B Rao wrote:
> Don't do CPU realization from cpu_generic_init(). With this
> cpu_generic_init() will be used to just create CPU threads and they
> should be realized separately from realizefn call.
>
> Convert the existing callers to do explicit realization.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed by comparing the patch with the results of the
following Coccinelle patch:
@@
typedef CPUState;
identifier cc, cpu, err;
@@
CPUState *cpu_generic_init(...)
{
...
- if (err != NULL) {
- goto out;
- }
-
- object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-out:
if (err != NULL) {
...
}
...
}
@@
type XXXCPU;
identifier initfunc, cpu_model, TYPE_XXX_CPU, XXX_CPU;
@@
XXXCPU *initfunc(const char *cpu_model)
{
- return XXX_CPU(cpu_generic_init(TYPE_XXX_CPU, cpu_model));
+ CPUState *cpu = cpu_generic_init(TYPE_XXX_CPU, cpu_model);
+ Error *err = NULL;
+
+ if (!cpu) {
+ return NULL;
+ }
+
+ object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+ if (err != NULL) {
+ error_report_err(err);
+ object_unref(OBJECT(cpu));
+ return NULL;
+ } else {
+ return XXX_CPU(cpu);
+ }
}
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (3 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 04/11] cpu: Don't realize CPU from cpu_generic_init() Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:13 ` David Gibson
2016-01-27 16:31 ` Matthew Rosato
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove() Bharata B Rao
` (5 subsequent siblings)
10 siblings, 2 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: Zhu Guihua, ehabkost, aik, Bharata B Rao, mdroth, agraf,
Chen Fan, pbonzini, qemu-ppc, tyreld, nfont, Gu Zheng, imammedo,
afaerber, david
From: Gu Zheng <guz.fnst@cn.fujitsu.com>
In order to deal well with the kvm vcpus (which can not be removed without any
protection), we do not close KVM vcpu fd, just record and mark it as stopped
into a list, so that we can reuse it for the appending cpu hot-add request if
possible. It is also the approach that kvm guys suggested:
https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
[- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
isn't needed as it is done from cpu_exec_exit()
- Use iothread mutex instead of global mutex during
destroy
- Don't cleanup vCPU object from vCPU thread context
but leave it to the callers (device_add/device_del)]
---
cpus.c | 38 +++++++++++++++++++++++++++++++++++
include/qom/cpu.h | 10 +++++++++
include/sysemu/kvm.h | 1 +
kvm-all.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
kvm-stub.c | 5 +++++
5 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/cpus.c b/cpus.c
index ea29584..12374af 100644
--- a/cpus.c
+++ b/cpus.c
@@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
qemu_cpu_kick(cpu);
}
+static void qemu_kvm_destroy_vcpu(CPUState *cpu)
+{
+ if (kvm_destroy_vcpu(cpu) < 0) {
+ error_report("kvm_destroy_vcpu failed.\n");
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void qemu_tcg_destroy_vcpu(CPUState *cpu)
+{
+}
+
static void flush_queued_work(CPUState *cpu)
{
struct qemu_work_item *wi;
@@ -1053,6 +1065,11 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
}
}
qemu_kvm_wait_io_event(cpu);
+ if (cpu->exit && !cpu_can_run(cpu)) {
+ qemu_kvm_destroy_vcpu(cpu);
+ qemu_mutex_unlock_iothread();
+ return NULL;
+ }
}
return NULL;
@@ -1108,6 +1125,7 @@ static void tcg_exec_all(void);
static void *qemu_tcg_cpu_thread_fn(void *arg)
{
CPUState *cpu = arg;
+ CPUState *remove_cpu = NULL;
rcu_register_thread();
@@ -1145,6 +1163,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
}
}
qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
+ CPU_FOREACH(cpu) {
+ if (cpu->exit && !cpu_can_run(cpu)) {
+ remove_cpu = cpu;
+ break;
+ }
+ }
+ if (remove_cpu) {
+ qemu_tcg_destroy_vcpu(remove_cpu);
+ remove_cpu = NULL;
+ }
}
return NULL;
@@ -1301,6 +1329,13 @@ void resume_all_vcpus(void)
}
}
+void cpu_remove(CPUState *cpu)
+{
+ cpu->stop = true;
+ cpu->exit = true;
+ qemu_cpu_kick(cpu);
+}
+
/* For temporary buffers for forming a name */
#define VCPU_THREAD_NAME_SIZE 16
@@ -1508,6 +1543,9 @@ static void tcg_exec_all(void)
break;
}
} else if (cpu->stop || cpu->stopped) {
+ if (cpu->exit) {
+ next_cpu = CPU_NEXT(cpu);
+ }
break;
}
}
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 51a1323..67e05b0 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -223,6 +223,7 @@ struct kvm_run;
* @halted: Nonzero if the CPU is in suspended state.
* @stop: Indicates a pending stop request.
* @stopped: Indicates the CPU has been artificially stopped.
+ * @exit: Indicates the CPU has exited due to an unplug operation.
* @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
* @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
* CPU and return to its top level loop.
@@ -274,6 +275,7 @@ struct CPUState {
bool created;
bool stop;
bool stopped;
+ bool exit;
bool crash_occurred;
bool exit_request;
uint32_t interrupt_request;
@@ -696,6 +698,14 @@ void cpu_exit(CPUState *cpu);
void cpu_resume(CPUState *cpu);
/**
+ * cpu_remove:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed.
+ */
+void cpu_remove(CPUState *cpu);
+
+/**
* qemu_init_vcpu:
* @cpu: The vCPU to initialize.
*
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 7741f91..7324fa9 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -218,6 +218,7 @@ int kvm_has_intx_set_mask(void);
int kvm_init_vcpu(CPUState *cpu);
int kvm_cpu_exec(CPUState *cpu);
+int kvm_destroy_vcpu(CPUState *cpu);
#ifdef NEED_CPU_H
diff --git a/kvm-all.c b/kvm-all.c
index bd9e764..b6f64ec 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -62,6 +62,12 @@
#define KVM_MSI_HASHTAB_SIZE 256
+struct KVMParkedVcpu {
+ unsigned long vcpu_id;
+ int kvm_fd;
+ QLIST_ENTRY(KVMParkedVcpu) node;
+};
+
struct KVMState
{
AccelState parent_obj;
@@ -95,6 +101,7 @@ struct KVMState
QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
#endif
KVMMemoryListener memory_listener;
+ QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
};
KVMState *kvm_state;
@@ -238,6 +245,53 @@ static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot)
return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
}
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+ KVMState *s = kvm_state;
+ long mmap_size;
+ struct KVMParkedVcpu *vcpu = NULL;
+ int ret = 0;
+
+ DPRINTF("kvm_destroy_vcpu\n");
+
+ mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
+ if (mmap_size < 0) {
+ ret = mmap_size;
+ DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n");
+ goto err;
+ }
+
+ ret = munmap(cpu->kvm_run, mmap_size);
+ if (ret < 0) {
+ goto err;
+ }
+
+ vcpu = g_malloc0(sizeof(*vcpu));
+ vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
+ vcpu->kvm_fd = cpu->kvm_fd;
+ QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+err:
+ return ret;
+}
+
+static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
+{
+ struct KVMParkedVcpu *cpu;
+
+ QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
+ if (cpu->vcpu_id == vcpu_id) {
+ int kvm_fd;
+
+ QLIST_REMOVE(cpu, node);
+ kvm_fd = cpu->kvm_fd;
+ g_free(cpu);
+ return kvm_fd;
+ }
+ }
+
+ return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
+}
+
int kvm_init_vcpu(CPUState *cpu)
{
KVMState *s = kvm_state;
@@ -246,7 +300,7 @@ int kvm_init_vcpu(CPUState *cpu)
DPRINTF("kvm_init_vcpu\n");
- ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)kvm_arch_vcpu_id(cpu));
+ ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
if (ret < 0) {
DPRINTF("kvm_create_vcpu failed\n");
goto err;
@@ -1509,6 +1563,7 @@ static int kvm_init(MachineState *ms)
#ifdef KVM_CAP_SET_GUEST_DEBUG
QTAILQ_INIT(&s->kvm_sw_breakpoints);
#endif
+ QLIST_INIT(&s->kvm_parked_vcpus);
s->vmfd = -1;
s->fd = qemu_open("/dev/kvm", O_RDWR);
if (s->fd == -1) {
diff --git a/kvm-stub.c b/kvm-stub.c
index dc97a5e..0b39456 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -32,6 +32,11 @@ bool kvm_allowed;
bool kvm_readonly_mem_allowed;
bool kvm_ioeventfd_any_length_allowed;
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+ return -ENOSYS;
+}
+
int kvm_init_vcpu(CPUState *cpu)
{
return -ENOSYS;
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects Bharata B Rao
@ 2016-01-12 4:13 ` David Gibson
2016-01-27 16:31 ` Matthew Rosato
1 sibling, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:13 UTC (permalink / raw)
To: Bharata B Rao
Cc: Zhu Guihua, mdroth, aik, agraf, qemu-devel, Chen Fan, pbonzini,
qemu-ppc, tyreld, nfont, Gu Zheng, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]
On Fri, Jan 08, 2016 at 12:25:13PM +0530, Bharata B Rao wrote:
> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>
> In order to deal well with the kvm vcpus (which can not be removed without any
> protection), we do not close KVM vcpu fd, just record and mark it as stopped
> into a list, so that we can reuse it for the appending cpu hot-add request if
> possible. It is also the approach that kvm guys suggested:
> https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
> isn't needed as it is done from cpu_exec_exit()
> - Use iothread mutex instead of global mutex during
> destroy
> - Don't cleanup vCPU object from vCPU thread context
> but leave it to the callers (device_add/device_del)]
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects Bharata B Rao
2016-01-12 4:13 ` David Gibson
@ 2016-01-27 16:31 ` Matthew Rosato
1 sibling, 0 replies; 39+ messages in thread
From: Matthew Rosato @ 2016-01-27 16:31 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel
Cc: Zhu Guihua, ehabkost, imammedo, aik, agraf, mdroth, Gu Zheng,
qemu-ppc, tyreld, nfont, Chen Fan, pbonzini, afaerber, david
On 01/08/2016 01:55 AM, Bharata B Rao wrote:
> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>
> In order to deal well with the kvm vcpus (which can not be removed without any
> protection), we do not close KVM vcpu fd, just record and mark it as stopped
> into a list, so that we can reuse it for the appending cpu hot-add request if
> possible. It is also the approach that kvm guys suggested:
> https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
> isn't needed as it is done from cpu_exec_exit()
> - Use iothread mutex instead of global mutex during
> destroy
> - Don't cleanup vCPU object from vCPU thread context
> but leave it to the callers (device_add/device_del)]
> ---
> cpus.c | 38 +++++++++++++++++++++++++++++++++++
> include/qom/cpu.h | 10 +++++++++
> include/sysemu/kvm.h | 1 +
> kvm-all.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> kvm-stub.c | 5 +++++
> 5 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/cpus.c b/cpus.c
> index ea29584..12374af 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
> qemu_cpu_kick(cpu);
> }
>
> +static void qemu_kvm_destroy_vcpu(CPUState *cpu)
> +{
> + if (kvm_destroy_vcpu(cpu) < 0) {
> + error_report("kvm_destroy_vcpu failed.\n");
FYI, checkpatch fails here -- no need for the newline.
Matt
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove()
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (4 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 05/11] cpu: Reclaim vCPU objects Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:16 ` David Gibson
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 07/11] xics, xics_kvm: Handle CPU unplug correctly Bharata B Rao
` (4 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
This sync API will be used by the CPU hotplug code to wait for the CPU to
completely get removed before flagging the failure to the device_add
command.
Sync version of this call is needed to correctly recover from CPU
realization failures when ->plug() handler fails.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
cpus.c | 12 ++++++++++++
include/qom/cpu.h | 8 ++++++++
2 files changed, 20 insertions(+)
diff --git a/cpus.c b/cpus.c
index 12374af..c829bd6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1067,6 +1067,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
qemu_kvm_wait_io_event(cpu);
if (cpu->exit && !cpu_can_run(cpu)) {
qemu_kvm_destroy_vcpu(cpu);
+ cpu->created = false;
+ qemu_cond_signal(&qemu_cpu_cond);
qemu_mutex_unlock_iothread();
return NULL;
}
@@ -1171,6 +1173,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
}
if (remove_cpu) {
qemu_tcg_destroy_vcpu(remove_cpu);
+ cpu->created = false;
+ qemu_cond_signal(&qemu_cpu_cond);
remove_cpu = NULL;
}
}
@@ -1336,6 +1340,14 @@ void cpu_remove(CPUState *cpu)
qemu_cpu_kick(cpu);
}
+void cpu_remove_sync(CPUState *cpu)
+{
+ cpu_remove(cpu);
+ while (cpu->created) {
+ qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+ }
+}
+
/* For temporary buffers for forming a name */
#define VCPU_THREAD_NAME_SIZE 16
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 67e05b0..7fc6696 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -705,6 +705,14 @@ void cpu_resume(CPUState *cpu);
*/
void cpu_remove(CPUState *cpu);
+ /**
+ * cpu_remove_sync:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed and waits till it is removed.
+ */
+void cpu_remove_sync(CPUState *cpu);
+
/**
* qemu_init_vcpu:
* @cpu: The vCPU to initialize.
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove()
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove() Bharata B Rao
@ 2016-01-12 4:16 ` David Gibson
2016-01-12 6:53 ` Bharata B Rao
0 siblings, 1 reply; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:16 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 2801 bytes --]
On Fri, Jan 08, 2016 at 12:25:14PM +0530, Bharata B Rao wrote:
> This sync API will be used by the CPU hotplug code to wait for the CPU to
> completely get removed before flagging the failure to the device_add
> command.
>
> Sync version of this call is needed to correctly recover from CPU
> realization failures when ->plug() handler fails.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> cpus.c | 12 ++++++++++++
> include/qom/cpu.h | 8 ++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index 12374af..c829bd6 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1067,6 +1067,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
> qemu_kvm_wait_io_event(cpu);
> if (cpu->exit && !cpu_can_run(cpu)) {
> qemu_kvm_destroy_vcpu(cpu);
> + cpu->created = false;
> + qemu_cond_signal(&qemu_cpu_cond);
I know I reviewed this earlier, but I suspect this needs to be
qemu_cond_broadcast. qemu_cpu_cond is global, so there could (at
least in theory) be multiple things waiting on it, and we don't know
which one is waiting on *this* cpu, rather than another one (or on any
other condition handled by this qemu_cpu_cond).
> qemu_mutex_unlock_iothread();
> return NULL;
> }
> @@ -1171,6 +1173,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
> }
> if (remove_cpu) {
> qemu_tcg_destroy_vcpu(remove_cpu);
> + cpu->created = false;
> + qemu_cond_signal(&qemu_cpu_cond);
> remove_cpu = NULL;
> }
> }
> @@ -1336,6 +1340,14 @@ void cpu_remove(CPUState *cpu)
> qemu_cpu_kick(cpu);
> }
>
> +void cpu_remove_sync(CPUState *cpu)
> +{
> + cpu_remove(cpu);
> + while (cpu->created) {
> + qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> + }
> +}
> +
> /* For temporary buffers for forming a name */
> #define VCPU_THREAD_NAME_SIZE 16
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 67e05b0..7fc6696 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -705,6 +705,14 @@ void cpu_resume(CPUState *cpu);
> */
> void cpu_remove(CPUState *cpu);
>
> + /**
> + * cpu_remove_sync:
> + * @cpu: The CPU to remove.
> + *
> + * Requests the CPU to be removed and waits till it is removed.
> + */
> +void cpu_remove_sync(CPUState *cpu);
> +
> /**
> * qemu_init_vcpu:
> * @cpu: The vCPU to initialize.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove()
2016-01-12 4:16 ` David Gibson
@ 2016-01-12 6:53 ` Bharata B Rao
2016-01-13 3:45 ` David Gibson
0 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-12 6:53 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
On Tue, Jan 12, 2016 at 03:16:15PM +1100, David Gibson wrote:
> On Fri, Jan 08, 2016 at 12:25:14PM +0530, Bharata B Rao wrote:
> > This sync API will be used by the CPU hotplug code to wait for the CPU to
> > completely get removed before flagging the failure to the device_add
> > command.
> >
> > Sync version of this call is needed to correctly recover from CPU
> > realization failures when ->plug() handler fails.
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > cpus.c | 12 ++++++++++++
> > include/qom/cpu.h | 8 ++++++++
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/cpus.c b/cpus.c
> > index 12374af..c829bd6 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1067,6 +1067,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
> > qemu_kvm_wait_io_event(cpu);
> > if (cpu->exit && !cpu_can_run(cpu)) {
> > qemu_kvm_destroy_vcpu(cpu);
> > + cpu->created = false;
> > + qemu_cond_signal(&qemu_cpu_cond);
>
> I know I reviewed this earlier, but I suspect this needs to be
> qemu_cond_broadcast. qemu_cpu_cond is global, so there could (at
> least in theory) be multiple things waiting on it, and we don't know
> which one is waiting on *this* cpu, rather than another one (or on any
> other condition handled by this qemu_cpu_cond).
qemu_cpu_cond condition variable is currently handling the CPU creation
condition where qemu_cond_signal() is being used. From what I understand,
only main thread will be waiting on this condition.
In this patch, I am using the same condition variable to track CPU
deletion condition too. After this too, I think only main thread will be
waiting for the deletion to get completed. So using qemu_cond_signal()
should be ok isn't ?
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove()
2016-01-12 6:53 ` Bharata B Rao
@ 2016-01-13 3:45 ` David Gibson
0 siblings, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-13 3:45 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]
On Tue, Jan 12, 2016 at 12:23:03PM +0530, Bharata B Rao wrote:
> On Tue, Jan 12, 2016 at 03:16:15PM +1100, David Gibson wrote:
> > On Fri, Jan 08, 2016 at 12:25:14PM +0530, Bharata B Rao wrote:
> > > This sync API will be used by the CPU hotplug code to wait for the CPU to
> > > completely get removed before flagging the failure to the device_add
> > > command.
> > >
> > > Sync version of this call is needed to correctly recover from CPU
> > > realization failures when ->plug() handler fails.
> > >
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > cpus.c | 12 ++++++++++++
> > > include/qom/cpu.h | 8 ++++++++
> > > 2 files changed, 20 insertions(+)
> > >
> > > diff --git a/cpus.c b/cpus.c
> > > index 12374af..c829bd6 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -1067,6 +1067,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
> > > qemu_kvm_wait_io_event(cpu);
> > > if (cpu->exit && !cpu_can_run(cpu)) {
> > > qemu_kvm_destroy_vcpu(cpu);
> > > + cpu->created = false;
> > > + qemu_cond_signal(&qemu_cpu_cond);
> >
> > I know I reviewed this earlier, but I suspect this needs to be
> > qemu_cond_broadcast. qemu_cpu_cond is global, so there could (at
> > least in theory) be multiple things waiting on it, and we don't know
> > which one is waiting on *this* cpu, rather than another one (or on any
> > other condition handled by this qemu_cpu_cond).
>
> qemu_cpu_cond condition variable is currently handling the CPU creation
> condition where qemu_cond_signal() is being used. From what I understand,
> only main thread will be waiting on this condition.
> In this patch, I am using the same condition variable to track CPU
> deletion condition too. After this too, I think only main thread will be
> waiting for the deletion to get completed. So using qemu_cond_signal()
> should be ok isn't ?
Ok, if it's just waking the main thread, qemu_cond_signal() should be
fine (in fact, equivalent to qemu_cond_broadcast()).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 07/11] xics, xics_kvm: Handle CPU unplug correctly
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (5 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove() Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:19 ` David Gibson
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device Bharata B Rao
` (3 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
XICS is setup for each CPU during initialization. Provide a routine
to undo the same when CPU is unplugged. While here, move ss->cs management
into xics from xics_kvm since there is nothing KVM specific in it.
Also ensure xics reset doesn't set irq for CPUs that are already unplugged.
This allows reboot of a VM that has undergone CPU hotplug and unplug
to work correctly.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/intc/xics.c | 13 +++++++++++++
hw/intc/xics_kvm.c | 8 ++++----
include/hw/ppc/xics.h | 1 +
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 9ff5796..37abaf4 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -44,6 +44,17 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
return -1;
}
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
+{
+ CPUState *cs = CPU(cpu);
+ ICPState *ss = &icp->ss[cs->cpu_index];
+
+ assert(cs->cpu_index < icp->nr_servers);
+
+ ss->output = NULL;
+ ss->cs = NULL;
+}
+
void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
{
CPUState *cs = CPU(cpu);
@@ -53,6 +64,8 @@ void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
assert(cs->cpu_index < icp->nr_servers);
+ ss->cs = cs;
+
if (info->cpu_setup) {
info->cpu_setup(icp, cpu);
}
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index d58729c..049d4e2 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -109,8 +109,10 @@ static void icp_kvm_reset(DeviceState *dev)
icp->pending_priority = 0xff;
icp->mfrr = 0xff;
- /* Make all outputs are deasserted */
- qemu_set_irq(icp->output, 0);
+ /* Make all outputs as deasserted only if the CPU thread is in use */
+ if (icp->output) {
+ qemu_set_irq(icp->output, 0);
+ }
icp_set_kvm_state(icp, 1);
}
@@ -343,8 +345,6 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
if (icpkvm->kernel_xics_fd != -1) {
int ret;
- ss->cs = cs;
-
ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0,
icpkvm->kernel_xics_fd, kvm_arch_vcpu_id(cs));
if (ret < 0) {
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 355a966..640162f 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -166,5 +166,6 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
void xics_free(XICSState *icp, int irq, int num);
void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
#endif /* __XICS_H__ */
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/11] xics, xics_kvm: Handle CPU unplug correctly
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 07/11] xics, xics_kvm: Handle CPU unplug correctly Bharata B Rao
@ 2016-01-12 4:19 ` David Gibson
0 siblings, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:19 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 3298 bytes --]
On Fri, Jan 08, 2016 at 12:25:15PM +0530, Bharata B Rao wrote:
> XICS is setup for each CPU during initialization. Provide a routine
> to undo the same when CPU is unplugged. While here, move ss->cs management
> into xics from xics_kvm since there is nothing KVM specific in it.
> Also ensure xics reset doesn't set irq for CPUs that are already unplugged.
>
> This allows reboot of a VM that has undergone CPU hotplug and unplug
> to work correctly.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Small suggestion below, but it's not of great importance.
> ---
> hw/intc/xics.c | 13 +++++++++++++
> hw/intc/xics_kvm.c | 8 ++++----
> include/hw/ppc/xics.h | 1 +
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 9ff5796..37abaf4 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -44,6 +44,17 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
> return -1;
> }
>
> +void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
> +{
> + CPUState *cs = CPU(cpu);
> + ICPState *ss = &icp->ss[cs->cpu_index];
> +
> + assert(cs->cpu_index < icp->nr_servers);
Maybe assert that ss->cs matches cs.
> +
> + ss->output = NULL;
> + ss->cs = NULL;
> +}
> +
> void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> {
> CPUState *cs = CPU(cpu);
> @@ -53,6 +64,8 @@ void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>
> assert(cs->cpu_index < icp->nr_servers);
>
> + ss->cs = cs;
> +
> if (info->cpu_setup) {
> info->cpu_setup(icp, cpu);
> }
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index d58729c..049d4e2 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -109,8 +109,10 @@ static void icp_kvm_reset(DeviceState *dev)
> icp->pending_priority = 0xff;
> icp->mfrr = 0xff;
>
> - /* Make all outputs are deasserted */
> - qemu_set_irq(icp->output, 0);
> + /* Make all outputs as deasserted only if the CPU thread is in use */
> + if (icp->output) {
> + qemu_set_irq(icp->output, 0);
> + }
>
> icp_set_kvm_state(icp, 1);
> }
> @@ -343,8 +345,6 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> if (icpkvm->kernel_xics_fd != -1) {
> int ret;
>
> - ss->cs = cs;
> -
> ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0,
> icpkvm->kernel_xics_fd, kvm_arch_vcpu_id(cs));
> if (ret < 0) {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 355a966..640162f 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -166,5 +166,6 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
> void xics_free(XICSState *icp, int irq, int num);
>
> void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
> +void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
>
> #endif /* __XICS_H__ */
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (6 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 07/11] xics, xics_kvm: Handle CPU unplug correctly Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 4:24 ` David Gibson
` (2 more replies)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 09/11] spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries Bharata B Rao
` (2 subsequent siblings)
10 siblings, 3 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
CPU core device is a container of CPU thread devices. CPU hotplug is
performed at the granularity of CPU core device. When hotplugged, CPU core
creates CPU thread devices.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/ppc/Makefile.objs | 1 +
hw/ppc/cpu-core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
include/hw/ppc/cpu-core.h | 22 +++++++++++++++
3 files changed, 92 insertions(+)
create mode 100644 hw/ppc/cpu-core.c
create mode 100644 include/hw/ppc/cpu-core.h
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index c1ffc77..a6b7cfb 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
# PowerPC 440 Xilinx ML507 reference board.
obj-$(CONFIG_XILINX) += virtex_ml507.o
+obj-y += cpu-core.o
diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
new file mode 100644
index 0000000..c5c6188
--- /dev/null
+++ b/hw/ppc/cpu-core.c
@@ -0,0 +1,69 @@
+/*
+ * CPU core device, acts as container of CPU thread devices.
+ *
+ * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "hw/ppc/cpu-core.h"
+#include "hw/boards.h"
+#include <sysemu/cpus.h>
+#include "qemu/error-report.h"
+
+static int ppc_cpu_core_realize_child(Object *child, void *opaque)
+{
+ Error **errp = opaque;
+
+ object_property_set_bool(child, true, "realized", errp);
+ if (*errp) {
+ return 1;
+ }
+
+ return 0;
+}
+
+static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
+{
+ object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
+}
+
+static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+
+ dc->realize = ppc_cpu_core_realize;
+}
+
+static void ppc_cpu_core_instance_init(Object *obj)
+{
+ int i;
+ CPUState *cpu;
+ MachineState *machine = MACHINE(qdev_get_machine());
+
+ /* Create as many CPU threads as specified in the topology */
+ for (i = 0; i < smp_threads; i++) {
+ cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
+ if (!cpu) {
+ error_report("Unable to find CPU definition: %s\n",
+ machine->cpu_model);
+ exit(EXIT_FAILURE);
+ }
+ object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
+ object_unref(OBJECT(cpu));
+ }
+}
+
+static const TypeInfo ppc_cpu_core_type_info = {
+ .name = TYPE_POWERPC_CPU_CORE,
+ .parent = TYPE_DEVICE,
+ .class_init = ppc_cpu_core_class_init,
+ .instance_init = ppc_cpu_core_instance_init,
+};
+
+static void cpu_core_register_types(void)
+{
+ type_register_static(&ppc_cpu_core_type_info);
+}
+
+type_init(cpu_core_register_types)
diff --git a/include/hw/ppc/cpu-core.h b/include/hw/ppc/cpu-core.h
new file mode 100644
index 0000000..91e31ef
--- /dev/null
+++ b/include/hw/ppc/cpu-core.h
@@ -0,0 +1,22 @@
+/*
+ * CPU core device.
+ *
+ * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef HW_PPC_CPU_CORE_H
+#define HW_PPC_CPU_CORE_H
+
+#include "hw/qdev.h"
+
+#ifdef TARGET_PPC64
+#define TYPE_POWERPC_CPU_CORE "powerpc64-cpu-core"
+#elif defined(TARGET_PPCEMB)
+#define TYPE_POWERPC_CPU_CORE "embedded-powerpc-cpu-core"
+#else
+#define TYPE_POWERPC_CPU_CORE "powerpc-cpu-core"
+#endif
+
+#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device Bharata B Rao
@ 2016-01-12 4:24 ` David Gibson
2016-01-12 23:30 ` Alexey Kardashevskiy
2016-01-12 23:44 ` Alexey Kardashevskiy
2 siblings, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 4:24 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 4862 bytes --]
On Fri, Jan 08, 2016 at 12:25:16PM +0530, Bharata B Rao wrote:
> CPU core device is a container of CPU thread devices. CPU hotplug is
> performed at the granularity of CPU core device. When hotplugged, CPU core
> creates CPU thread devices.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Some comments below, but nothing serious enough that I'd delay the
patch for it.
> ---
> hw/ppc/Makefile.objs | 1 +
> hw/ppc/cpu-core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/cpu-core.h | 22 +++++++++++++++
> 3 files changed, 92 insertions(+)
> create mode 100644 hw/ppc/cpu-core.c
> create mode 100644 include/hw/ppc/cpu-core.h
>
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..a6b7cfb 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
> obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
> # PowerPC 440 Xilinx ML507 reference board.
> obj-$(CONFIG_XILINX) += virtex_ml507.o
> +obj-y += cpu-core.o
> diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> new file mode 100644
> index 0000000..c5c6188
> --- /dev/null
> +++ b/hw/ppc/cpu-core.c
> @@ -0,0 +1,69 @@
> +/*
> + * CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/ppc/cpu-core.h"
> +#include "hw/boards.h"
> +#include <sysemu/cpus.h>
> +#include "qemu/error-report.h"
> +
> +static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> +{
> + Error **errp = opaque;
> +
> + object_property_set_bool(child, true, "realized", errp);
> + if (*errp) {
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> + object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
> +}
> +
> +static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = ppc_cpu_core_realize;
> +}
> +
> +static void ppc_cpu_core_instance_init(Object *obj)
> +{
> + int i;
> + CPUState *cpu;
> + MachineState *machine = MACHINE(qdev_get_machine());
> +
> + /* Create as many CPU threads as specified in the topology */
> + for (i = 0; i < smp_threads; i++) {
I think it would make more sense to have a field in the core object
give the number of threads, even if it's just initialized to global
smp_threads for the time being.
> + cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
> + if (!cpu) {
> + error_report("Unable to find CPU definition: %s\n",
> + machine->cpu_model);
> + exit(EXIT_FAILURE);
> + }
> + object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
Using thread[*] is a bit nasty, since we already know the correct
index. But I guess it avoids some messy string mangling, and it's not
like there's enough threads to make list[*]'s O(N^2) behaviour to be a
real problem.
> + object_unref(OBJECT(cpu));
> + }
> +}
> +
> +static const TypeInfo ppc_cpu_core_type_info = {
> + .name = TYPE_POWERPC_CPU_CORE,
> + .parent = TYPE_DEVICE,
> + .class_init = ppc_cpu_core_class_init,
> + .instance_init = ppc_cpu_core_instance_init,
> +};
> +
> +static void cpu_core_register_types(void)
> +{
> + type_register_static(&ppc_cpu_core_type_info);
> +}
> +
> +type_init(cpu_core_register_types)
> diff --git a/include/hw/ppc/cpu-core.h b/include/hw/ppc/cpu-core.h
> new file mode 100644
> index 0000000..91e31ef
> --- /dev/null
> +++ b/include/hw/ppc/cpu-core.h
> @@ -0,0 +1,22 @@
> +/*
> + * CPU core device.
> + *
> + * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_PPC_CPU_CORE_H
> +#define HW_PPC_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +
> +#ifdef TARGET_PPC64
> +#define TYPE_POWERPC_CPU_CORE "powerpc64-cpu-core"
> +#elif defined(TARGET_PPCEMB)
> +#define TYPE_POWERPC_CPU_CORE "embedded-powerpc-cpu-core"
> +#else
> +#define TYPE_POWERPC_CPU_CORE "powerpc-cpu-core"
> +#endif
> +
> +#endif
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device Bharata B Rao
2016-01-12 4:24 ` David Gibson
@ 2016-01-12 23:30 ` Alexey Kardashevskiy
2016-01-12 23:44 ` Alexey Kardashevskiy
2 siblings, 0 replies; 39+ messages in thread
From: Alexey Kardashevskiy @ 2016-01-12 23:30 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel
Cc: ehabkost, mdroth, agraf, pbonzini, qemu-ppc, tyreld, nfont,
imammedo, afaerber, david
On 01/08/2016 05:55 PM, Bharata B Rao wrote:
> CPU core device is a container of CPU thread devices. CPU hotplug is
> performed at the granularity of CPU core device. When hotplugged, CPU core
> creates CPU thread devices.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/ppc/Makefile.objs | 1 +
> hw/ppc/cpu-core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/cpu-core.h | 22 +++++++++++++++
> 3 files changed, 92 insertions(+)
> create mode 100644 hw/ppc/cpu-core.c
> create mode 100644 include/hw/ppc/cpu-core.h
>
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..a6b7cfb 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
> obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
> # PowerPC 440 Xilinx ML507 reference board.
> obj-$(CONFIG_XILINX) += virtex_ml507.o
> +obj-y += cpu-core.o
> diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> new file mode 100644
> index 0000000..c5c6188
> --- /dev/null
> +++ b/hw/ppc/cpu-core.c
> @@ -0,0 +1,69 @@
> +/*
> + * CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
2016? :)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/ppc/cpu-core.h"
> +#include "hw/boards.h"
> +#include <sysemu/cpus.h>
> +#include "qemu/error-report.h"
> +
> +static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> +{
> + Error **errp = opaque;
> +
> + object_property_set_bool(child, true, "realized", errp);
> + if (*errp) {
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> + object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
> +}
> +
> +static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = ppc_cpu_core_realize;
> +}
> +
> +static void ppc_cpu_core_instance_init(Object *obj)
> +{
> + int i;
> + CPUState *cpu;
> + MachineState *machine = MACHINE(qdev_get_machine());
> +
> + /* Create as many CPU threads as specified in the topology */
> + for (i = 0; i < smp_threads; i++) {
> + cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
> + if (!cpu) {
> + error_report("Unable to find CPU definition: %s\n",
> + machine->cpu_model);
> + exit(EXIT_FAILURE);
> + }
> + object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
> + object_unref(OBJECT(cpu));
> + }
> +}
> +
> +static const TypeInfo ppc_cpu_core_type_info = {
> + .name = TYPE_POWERPC_CPU_CORE,
> + .parent = TYPE_DEVICE,
> + .class_init = ppc_cpu_core_class_init,
> + .instance_init = ppc_cpu_core_instance_init,
> +};
> +
> +static void cpu_core_register_types(void)
> +{
> + type_register_static(&ppc_cpu_core_type_info);
> +}
> +
> +type_init(cpu_core_register_types)
> diff --git a/include/hw/ppc/cpu-core.h b/include/hw/ppc/cpu-core.h
> new file mode 100644
> index 0000000..91e31ef
> --- /dev/null
> +++ b/include/hw/ppc/cpu-core.h
> @@ -0,0 +1,22 @@
> +/*
> + * CPU core device.
> + *
> + * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_PPC_CPU_CORE_H
> +#define HW_PPC_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +
> +#ifdef TARGET_PPC64
> +#define TYPE_POWERPC_CPU_CORE "powerpc64-cpu-core"
> +#elif defined(TARGET_PPCEMB)
> +#define TYPE_POWERPC_CPU_CORE "embedded-powerpc-cpu-core"
> +#else
> +#define TYPE_POWERPC_CPU_CORE "powerpc-cpu-core"
> +#endif
> +
> +#endif
>
--
Alexey
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device Bharata B Rao
2016-01-12 4:24 ` David Gibson
2016-01-12 23:30 ` Alexey Kardashevskiy
@ 2016-01-12 23:44 ` Alexey Kardashevskiy
2016-01-13 4:30 ` Bharata B Rao
2 siblings, 1 reply; 39+ messages in thread
From: Alexey Kardashevskiy @ 2016-01-12 23:44 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel
Cc: ehabkost, mdroth, agraf, pbonzini, qemu-ppc, tyreld, nfont,
imammedo, afaerber, david
On 01/08/2016 05:55 PM, Bharata B Rao wrote:
> CPU core device is a container of CPU thread devices. CPU hotplug is
> performed at the granularity of CPU core device. When hotplugged, CPU core
> creates CPU thread devices.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/ppc/Makefile.objs | 1 +
> hw/ppc/cpu-core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/cpu-core.h | 22 +++++++++++++++
> 3 files changed, 92 insertions(+)
> create mode 100644 hw/ppc/cpu-core.c
> create mode 100644 include/hw/ppc/cpu-core.h
>
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..a6b7cfb 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
> obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
> # PowerPC 440 Xilinx ML507 reference board.
> obj-$(CONFIG_XILINX) += virtex_ml507.o
> +obj-y += cpu-core.o
> diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> new file mode 100644
> index 0000000..c5c6188
> --- /dev/null
> +++ b/hw/ppc/cpu-core.c
> @@ -0,0 +1,69 @@
> +/*
> + * CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/ppc/cpu-core.h"
> +#include "hw/boards.h"
> +#include <sysemu/cpus.h>
> +#include "qemu/error-report.h"
> +
> +static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> +{
> + Error **errp = opaque;
> +
> + object_property_set_bool(child, true, "realized", errp);
> + if (*errp) {
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> + object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
> +}
> +
> +static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = ppc_cpu_core_realize;
> +}
> +
> +static void ppc_cpu_core_instance_init(Object *obj)
> +{
> + int i;
> + CPUState *cpu;
> + MachineState *machine = MACHINE(qdev_get_machine());
> +
> + /* Create as many CPU threads as specified in the topology */
> + for (i = 0; i < smp_threads; i++) {
> + cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
> + if (!cpu) {
> + error_report("Unable to find CPU definition: %s\n",
> + machine->cpu_model);
> + exit(EXIT_FAILURE);
> + }
> + object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
> + object_unref(OBJECT(cpu));
> + }
> +}
> +
> +static const TypeInfo ppc_cpu_core_type_info = {
> + .name = TYPE_POWERPC_CPU_CORE,
> + .parent = TYPE_DEVICE,
> + .class_init = ppc_cpu_core_class_init,
> + .instance_init = ppc_cpu_core_instance_init,
Out of curiosity - why not .realize (and return Error instead of exit())?
I'd think this is the recommended approach now for QOM.
--
Alexey
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device
2016-01-12 23:44 ` Alexey Kardashevskiy
@ 2016-01-13 4:30 ` Bharata B Rao
0 siblings, 0 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-13 4:30 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: mdroth, agraf, ehabkost, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, david
On Wed, Jan 13, 2016 at 10:44:07AM +1100, Alexey Kardashevskiy wrote:
> On 01/08/2016 05:55 PM, Bharata B Rao wrote:
> >CPU core device is a container of CPU thread devices. CPU hotplug is
> >performed at the granularity of CPU core device. When hotplugged, CPU core
> >creates CPU thread devices.
> >
> >Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >---
> > hw/ppc/Makefile.objs | 1 +
> > hw/ppc/cpu-core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/cpu-core.h | 22 +++++++++++++++
> > 3 files changed, 92 insertions(+)
> > create mode 100644 hw/ppc/cpu-core.c
> > create mode 100644 include/hw/ppc/cpu-core.h
> >
> >diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >index c1ffc77..a6b7cfb 100644
> >--- a/hw/ppc/Makefile.objs
> >+++ b/hw/ppc/Makefile.objs
> >@@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
> > obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
> > # PowerPC 440 Xilinx ML507 reference board.
> > obj-$(CONFIG_XILINX) += virtex_ml507.o
> >+obj-y += cpu-core.o
> >diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> >new file mode 100644
> >index 0000000..c5c6188
> >--- /dev/null
> >+++ b/hw/ppc/cpu-core.c
> >@@ -0,0 +1,69 @@
> >+/*
> >+ * CPU core device, acts as container of CPU thread devices.
> >+ *
> >+ * Copyright (C) 2015 Bharata B Rao <bharata@linux.vnet.ibm.com>
> >+ *
> >+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >+ * See the COPYING file in the top-level directory.
> >+ */
> >+#include "hw/ppc/cpu-core.h"
> >+#include "hw/boards.h"
> >+#include <sysemu/cpus.h>
> >+#include "qemu/error-report.h"
> >+
> >+static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> >+{
> >+ Error **errp = opaque;
> >+
> >+ object_property_set_bool(child, true, "realized", errp);
> >+ if (*errp) {
> >+ return 1;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
> >+{
> >+ object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
> >+}
> >+
> >+static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
> >+{
> >+ DeviceClass *dc = DEVICE_CLASS(oc);
> >+
> >+ dc->realize = ppc_cpu_core_realize;
> >+}
> >+
> >+static void ppc_cpu_core_instance_init(Object *obj)
> >+{
> >+ int i;
> >+ CPUState *cpu;
> >+ MachineState *machine = MACHINE(qdev_get_machine());
> >+
> >+ /* Create as many CPU threads as specified in the topology */
> >+ for (i = 0; i < smp_threads; i++) {
> >+ cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
> >+ if (!cpu) {
> >+ error_report("Unable to find CPU definition: %s\n",
> >+ machine->cpu_model);
> >+ exit(EXIT_FAILURE);
> >+ }
> >+ object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
> >+ object_unref(OBJECT(cpu));
> >+ }
> >+}
> >+
> >+static const TypeInfo ppc_cpu_core_type_info = {
> >+ .name = TYPE_POWERPC_CPU_CORE,
> >+ .parent = TYPE_DEVICE,
> >+ .class_init = ppc_cpu_core_class_init,
> >+ .instance_init = ppc_cpu_core_instance_init,
>
>
> Out of curiosity - why not .realize (and return Error instead of exit())?
> I'd think this is the recommended approach now for QOM.
This is an error during object creation and hence we don't have any
other option but to exit. I guess I should use error_setg(&error_fatal, )
instead of exit().
Note that realize time errors are being handled in
.realize (ppc_cpu_core_realize) already.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 09/11] spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (7 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 08/11] target-ppc: Introduce PowerPC specific CPU core device Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 5:41 ` David Gibson
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support Bharata B Rao
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support Bharata B Rao
10 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
Start supporting CPU hotplug from pseries-2.6 onwards. Add CPU
DRC (Dynamic Resource Connector) device tree entries.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/ppc/spapr.c | 23 +++++++++++++++++++++++
include/hw/ppc/spapr.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a330169..a3ce1db 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -977,6 +977,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
_FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
}
+ if (smc->dr_cpu_enabled) {
+ int offset = fdt_path_offset(fdt, "/cpus");
+ ret = spapr_drc_populate_dt(fdt, offset, NULL,
+ SPAPR_DR_CONNECTOR_TYPE_CPU);
+ if (ret < 0) {
+ fprintf(stderr, "Couldn't set up CPU DR device tree properties\n");
+ exit(1);
+ }
+ }
+
_FDT((fdt_pack(fdt)));
if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
@@ -1731,6 +1741,8 @@ static void ppc_spapr_init(MachineState *machine)
long load_limit, fw_size;
bool kernel_le = false;
char *filename;
+ int smt = kvmppc_smt_threads();
+ int smp_max_cores = max_cpus/smp_threads;
msi_supported = true;
@@ -1797,6 +1809,15 @@ static void ppc_spapr_init(MachineState *machine)
spapr_validate_node_memory(machine);
}
+ if (smc->dr_cpu_enabled) {
+ for (i = 0; i < smp_max_cores; i++) {
+ sPAPRDRConnector *drc =
+ spapr_dr_connector_new(OBJECT(spapr),
+ SPAPR_DR_CONNECTOR_TYPE_CPU, i * smt);
+ qemu_register_reset(spapr_drc_reset, drc);
+ }
+ }
+
/* init CPUs */
if (machine->cpu_model == NULL) {
machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
@@ -2299,6 +2320,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
smc->dr_lmb_enabled = true;
+ smc->dr_cpu_enabled = true;
fwc->get_dev_path = spapr_get_fw_dev_path;
nc->nmi_monitor_handler = spapr_nmi;
}
@@ -2384,6 +2406,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
spapr_machine_2_6_class_options(mc);
smc->use_ohci_by_default = true;
+ smc->dr_cpu_enabled = false;
SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
mc->validate_smp_config = spapr_validate_smp_config_default;
}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 53af76a..739f9ba 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -36,6 +36,7 @@ struct sPAPRMachineClass {
/*< public >*/
bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
+ bool dr_cpu_enabled; /* enable dynamic-reconfig/hotplug of CPUs */
bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
};
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/11] spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 09/11] spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries Bharata B Rao
@ 2016-01-12 5:41 ` David Gibson
0 siblings, 0 replies; 39+ messages in thread
From: David Gibson @ 2016-01-12 5:41 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 3507 bytes --]
On Fri, Jan 08, 2016 at 12:25:17PM +0530, Bharata B Rao wrote:
> Start supporting CPU hotplug from pseries-2.6 onwards. Add CPU
> DRC (Dynamic Resource Connector) device tree entries.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 23 +++++++++++++++++++++++
> include/hw/ppc/spapr.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a330169..a3ce1db 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -977,6 +977,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> }
>
> + if (smc->dr_cpu_enabled) {
> + int offset = fdt_path_offset(fdt, "/cpus");
> + ret = spapr_drc_populate_dt(fdt, offset, NULL,
> + SPAPR_DR_CONNECTOR_TYPE_CPU);
> + if (ret < 0) {
> + fprintf(stderr, "Couldn't set up CPU DR device tree properties\n");
> + exit(1);
> + }
> + }
> +
> _FDT((fdt_pack(fdt)));
>
> if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> @@ -1731,6 +1741,8 @@ static void ppc_spapr_init(MachineState *machine)
> long load_limit, fw_size;
> bool kernel_le = false;
> char *filename;
> + int smt = kvmppc_smt_threads();
> + int smp_max_cores = max_cpus/smp_threads;
>
> msi_supported = true;
>
> @@ -1797,6 +1809,15 @@ static void ppc_spapr_init(MachineState *machine)
> spapr_validate_node_memory(machine);
> }
>
> + if (smc->dr_cpu_enabled) {
> + for (i = 0; i < smp_max_cores; i++) {
> + sPAPRDRConnector *drc =
> + spapr_dr_connector_new(OBJECT(spapr),
> + SPAPR_DR_CONNECTOR_TYPE_CPU, i * smt);
> + qemu_register_reset(spapr_drc_reset, drc);
> + }
> + }
> +
> /* init CPUs */
> if (machine->cpu_model == NULL) {
> machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> @@ -2299,6 +2320,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
>
> smc->dr_lmb_enabled = true;
> + smc->dr_cpu_enabled = true;
> fwc->get_dev_path = spapr_get_fw_dev_path;
> nc->nmi_monitor_handler = spapr_nmi;
> }
> @@ -2384,6 +2406,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
>
> spapr_machine_2_6_class_options(mc);
> smc->use_ohci_by_default = true;
> + smc->dr_cpu_enabled = false;
> SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> mc->validate_smp_config = spapr_validate_smp_config_default;
> }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 53af76a..739f9ba 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
>
> /*< public >*/
> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> + bool dr_cpu_enabled; /* enable dynamic-reconfig/hotplug of CPUs */
> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> };
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (8 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 09/11] spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 5:58 ` David Gibson
2016-01-12 23:58 ` Alexey Kardashevskiy
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support Bharata B Rao
10 siblings, 2 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
Support CPU hotplug via device-add command like this:
(qemu) device_add powerpc64-cpu-core,id=core2
In response to device_add, CPU core device will be created. CPU core
device creates and realizes CPU thread devices. If the machine type
supports CPU hotplug, boot-time CPUs are created as CPU core devices
otherwise they continue to be created as individual CPU devices.
Set up device tree entries for the hotplugged CPU core and use the
exising EPOW event infrastructure to send CPU hotplug notification to
the guest.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/ppc/spapr.c | 183 ++++++++++++++++++++++++++++++++++++++++++--
hw/ppc/spapr_events.c | 3 +
hw/ppc/spapr_rtas.c | 24 ++++++
include/hw/ppc/spapr.h | 5 ++
target-ppc/translate_init.c | 8 ++
5 files changed, 216 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a3ce1db..c2af9ca 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -63,6 +63,7 @@
#include "hw/compat.h"
#include "qemu-common.h"
+#include "hw/ppc/cpu-core.h"
#include <libfdt.h>
@@ -600,6 +601,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
size_t page_sizes_prop_size;
uint32_t vcpus_per_socket = smp_threads * smp_cores;
uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+ sPAPRDRConnector *drc;
+ sPAPRDRConnectorClass *drck;
+ int drc_index;
+
+ if (smc->dr_cpu_enabled) {
+ drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
+ g_assert(drc);
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ drc_index = drck->get_index(drc);
+ _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
+ }
/* Note: we keep CI large pages off for now because a 64K capable guest
* provisioned with large pages might otherwise try to map a qemu
@@ -1743,6 +1756,8 @@ static void ppc_spapr_init(MachineState *machine)
char *filename;
int smt = kvmppc_smt_threads();
int smp_max_cores = max_cpus/smp_threads;
+ int spapr_smp_cores = smp_cpus/smp_threads;
+ Object *core;
msi_supported = true;
@@ -1822,13 +1837,22 @@ static void ppc_spapr_init(MachineState *machine)
if (machine->cpu_model == NULL) {
machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
}
- for (i = 0; i < smp_cpus; i++) {
- cpu = cpu_ppc_init(machine->cpu_model);
- if (cpu == NULL) {
- fprintf(stderr, "Unable to find PowerPC CPU definition\n");
- exit(1);
+
+ if (smc->dr_cpu_enabled) {
+ for (i = 0; i < spapr_smp_cores; i++) {
+ core = object_new(TYPE_POWERPC_CPU_CORE);
+ object_property_set_bool(core, true, "realized", &error_abort);
+ }
+ } else {
+ for (i = 0; i < smp_cpus; i++) {
+ cpu = cpu_ppc_init(machine->cpu_model);
+ if (cpu == NULL) {
+ fprintf(stderr, "Unable to find PowerPC CPU definition\n");
+ exit(1);
+ }
+ object_property_set_bool(OBJECT(cpu), true, "realized",
+ &error_abort);
}
- spapr_cpu_init(spapr, cpu);
}
if (kvm_enabled()) {
@@ -2222,10 +2246,125 @@ out:
error_propagate(errp, local_err);
}
+static void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
+ int *fdt_offset,
+ sPAPRMachineState *spapr)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ DeviceClass *dc = DEVICE_GET_CLASS(cs);
+ int id = ppc_get_vcpu_dt_id(cpu);
+ void *fdt;
+ int offset, fdt_size;
+ char *nodename;
+
+ fdt = create_device_tree(&fdt_size);
+ nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
+ offset = fdt_add_subnode(fdt, 0, nodename);
+
+ spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+ g_free(nodename);
+
+ *fdt_offset = offset;
+ return fdt;
+}
+
+static int spapr_core_attach(Object *obj, void *opaque)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+ sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine());
+ sPAPRCoreState *core = opaque;
+ DeviceState *dev = DEVICE(obj);
+ CPUState *cs = CPU(dev);
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ int id = ppc_get_vcpu_dt_id(cpu);
+ sPAPRDRConnector *drc =
+ spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
+ sPAPRDRConnectorClass *drck;
+ int smt = kvmppc_smt_threads();
+ Error *local_err = NULL;
+ void *fdt = NULL;
+ int fdt_offset = 0;
+
+ /*
+ * Only main SMT thread (thread 0) will continue and signal the
+ * hotplug event to the guest. Other threads of the core will
+ * return from here.
+ */
+ if ((id % smt) != 0) {
+ return 0;
+ }
+
+ if (!smc->dr_cpu_enabled) {
+ /*
+ * This is a cold plugged CPU but the machine doesn't support
+ * DR. So skip the hotplug path ensuring that the CPU is brought
+ * up online with out an associated DR connector.
+ */
+ return 0;
+ }
+
+ g_assert(drc);
+
+ /*
+ * Setup CPU DT entries only for hotplugged CPUs. For boot time or
+ * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
+ */
+ if (dev->hotplugged) {
+ fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
+ }
+
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ drck->attach(drc, core->dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
+ if (local_err) {
+ g_free(fdt);
+ error_propagate(core->errp, local_err);
+ return 1;
+ }
+
+ /*
+ * We send hotplug notification interrupt to the guest only in case
+ * of hotplugged CPUs.
+ */
+ if (dev->hotplugged) {
+ spapr_hotplug_req_add_by_index(drc);
+ } else {
+ /*
+ * HACK to support removal of hotplugged CPU after VM migration:
+ *
+ * Since we want to be able to hot-remove those coldplugged CPUs
+ * started at boot time using -device option at the target VM, we set
+ * the right allocation_state and isolation_state for them, which for
+ * the hotplugged CPUs would be set via RTAS calls done from the
+ * guest during hotplug.
+ *
+ * This allows the coldplugged CPUs started using -device option to
+ * have the right isolation and allocation states as expected by the
+ * CPU hot removal code.
+ *
+ * This hack will be removed once we have DRC states migrated as part
+ * of VM migration.
+ */
+ drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
+ drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
+ }
+ return 0;
+}
+
+static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+ sPAPRCoreState core;
+
+ core.dev = dev;
+ core.errp = errp;
+ object_child_foreach(OBJECT(dev), spapr_core_attach, &core);
+}
+
static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+ sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
int node;
@@ -2262,6 +2401,34 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
}
spapr_memory_plug(hotplug_dev, dev, node, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+ CPUState *cs = CPU(dev);
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ int i;
+
+ /* Set NUMA node for the added CPUs */
+ for (i = 0; i < nb_numa_nodes; i++) {
+ if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
+ cs->numa_node = i;
+ break;
+ }
+ }
+
+ if (!smc->dr_cpu_enabled) {
+ if (dev->hotplugged) {
+ error_setg(errp, "CPU hotplug not supported for this machine");
+ cpu_remove_sync(cs);
+ return;
+ } else {
+ spapr_cpu_init(ms, cpu);
+ return;
+ }
+ }
+
+ spapr_cpu_init(ms, cpu);
+ spapr_cpu_reset(cpu);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
+ spapr_core_plug(hotplug_dev, dev, errp);
}
}
@@ -2276,7 +2443,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
DeviceState *dev)
{
- if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+ object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
+ object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
return HOTPLUG_HANDLER(machine);
}
return NULL;
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 744ea62..1063036 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -436,6 +436,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
case SPAPR_DR_CONNECTOR_TYPE_LMB:
hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
break;
+ case SPAPR_DR_CONNECTOR_TYPE_CPU:
+ hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
+ break;
default:
/* we shouldn't be signaling hotplug events for resources
* that don't support them
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 34b12a3..7baa862 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -33,6 +33,7 @@
#include "hw/ppc/spapr.h"
#include "hw/ppc/spapr_vio.h"
+#include "hw/ppc/ppc.h"
#include "qapi-event.h"
#include "hw/boards.h"
@@ -159,6 +160,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
}
+/*
+ * Set the timebase offset of the CPU to that of first CPU.
+ * This helps hotplugged CPU to have the correct timebase offset.
+ */
+static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
+{
+ PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
+
+ cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
+}
+
+static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
+{
+ PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
+ PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
+
+ if (!pcc->interrupts_big_endian(fcpu)) {
+ cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
+ }
+}
+
static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
uint32_t token, uint32_t nargs,
target_ulong args,
@@ -195,6 +217,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
env->nip = start;
env->gpr[3] = r3;
cs->halted = 0;
+ spapr_cpu_set_endianness(cpu);
+ spapr_cpu_update_tb_offset(cpu);
qemu_cpu_kick(cs);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 739f9ba..68d51d6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -83,6 +83,11 @@ struct sPAPRMachineState {
MemoryHotplugState hotplug_memory;
};
+typedef struct sPAPRCoreState {
+ DeviceState *dev;
+ Error **errp;
+} sPAPRCoreState;
+
#define H_SUCCESS 0
#define H_BUSY 1 /* Hardware busy -- retry later */
#define H_CLOSED 2 /* Resource closed */
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index d5ae53e..651dd41 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -30,6 +30,9 @@
#include "qemu/error-report.h"
#include "qapi/visitor.h"
#include "hw/qdev-properties.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "sysemu/sysemu.h"
+#endif
//#define PPC_DUMP_CPU
//#define PPC_DEBUG_SPR
@@ -8933,6 +8936,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
}
#if !defined(CONFIG_USER_ONLY)
+ if (cs->cpu_index >= max_cpus) {
+ error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
+ return;
+ }
+
cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
+ (cs->cpu_index % smp_threads);
#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support Bharata B Rao
@ 2016-01-12 5:58 ` David Gibson
2016-01-13 3:55 ` Bharata B Rao
2016-01-12 23:58 ` Alexey Kardashevskiy
1 sibling, 1 reply; 39+ messages in thread
From: David Gibson @ 2016-01-12 5:58 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 14482 bytes --]
On Fri, Jan 08, 2016 at 12:25:18PM +0530, Bharata B Rao wrote:
> Support CPU hotplug via device-add command like this:
>
> (qemu) device_add powerpc64-cpu-core,id=core2
>
> In response to device_add, CPU core device will be created. CPU core
> device creates and realizes CPU thread devices. If the machine type
> supports CPU hotplug, boot-time CPUs are created as CPU core devices
> otherwise they continue to be created as individual CPU devices.
>
> Set up device tree entries for the hotplugged CPU core and use the
> exising EPOW event infrastructure to send CPU hotplug notification to
> the guest.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 183 ++++++++++++++++++++++++++++++++++++++++++--
> hw/ppc/spapr_events.c | 3 +
> hw/ppc/spapr_rtas.c | 24 ++++++
> include/hw/ppc/spapr.h | 5 ++
> target-ppc/translate_init.c | 8 ++
> 5 files changed, 216 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a3ce1db..c2af9ca 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -63,6 +63,7 @@
>
> #include "hw/compat.h"
> #include "qemu-common.h"
> +#include "hw/ppc/cpu-core.h"
>
> #include <libfdt.h>
>
> @@ -600,6 +601,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> size_t page_sizes_prop_size;
> uint32_t vcpus_per_socket = smp_threads * smp_cores;
> uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> + sPAPRDRConnector *drc;
> + sPAPRDRConnectorClass *drck;
> + int drc_index;
> +
> + if (smc->dr_cpu_enabled) {
> + drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> + g_assert(drc);
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + drc_index = drck->get_index(drc);
> + _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> + }
>
> /* Note: we keep CI large pages off for now because a 64K capable guest
> * provisioned with large pages might otherwise try to map a qemu
> @@ -1743,6 +1756,8 @@ static void ppc_spapr_init(MachineState *machine)
> char *filename;
> int smt = kvmppc_smt_threads();
> int smp_max_cores = max_cpus/smp_threads;
> + int spapr_smp_cores = smp_cpus/smp_threads;
> + Object *core;
>
> msi_supported = true;
>
> @@ -1822,13 +1837,22 @@ static void ppc_spapr_init(MachineState *machine)
> if (machine->cpu_model == NULL) {
> machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> }
> - for (i = 0; i < smp_cpus; i++) {
> - cpu = cpu_ppc_init(machine->cpu_model);
> - if (cpu == NULL) {
> - fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> - exit(1);
> +
> + if (smc->dr_cpu_enabled) {
> + for (i = 0; i < spapr_smp_cores; i++) {
> + core = object_new(TYPE_POWERPC_CPU_CORE);
> + object_property_set_bool(core, true, "realized", &error_abort);
> + }
> + } else {
> + for (i = 0; i < smp_cpus; i++) {
> + cpu = cpu_ppc_init(machine->cpu_model);
> + if (cpu == NULL) {
> + fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> + exit(1);
> + }
> + object_property_set_bool(OBJECT(cpu), true, "realized",
> + &error_abort);
> }
> - spapr_cpu_init(spapr, cpu);
> }
>
> if (kvm_enabled()) {
> @@ -2222,10 +2246,125 @@ out:
> error_propagate(errp, local_err);
> }
>
> +static void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> + int *fdt_offset,
> + sPAPRMachineState *spapr)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + DeviceClass *dc = DEVICE_GET_CLASS(cs);
> + int id = ppc_get_vcpu_dt_id(cpu);
> + void *fdt;
> + int offset, fdt_size;
> + char *nodename;
> +
> + fdt = create_device_tree(&fdt_size);
> + nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
> + offset = fdt_add_subnode(fdt, 0, nodename);
> +
> + spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> + g_free(nodename);
> +
> + *fdt_offset = offset;
> + return fdt;
> +}
> +
> +static int spapr_core_attach(Object *obj, void *opaque)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> + sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine());
> + sPAPRCoreState *core = opaque;
> + DeviceState *dev = DEVICE(obj);
> + CPUState *cs = CPU(dev);
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + int id = ppc_get_vcpu_dt_id(cpu);
> + sPAPRDRConnector *drc =
> + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> + sPAPRDRConnectorClass *drck;
> + int smt = kvmppc_smt_threads();
> + Error *local_err = NULL;
> + void *fdt = NULL;
> + int fdt_offset = 0;
> +
> + /*
> + * Only main SMT thread (thread 0) will continue and signal the
> + * hotplug event to the guest. Other threads of the core will
> + * return from here.
> + */
> + if ((id % smt) != 0) {
> + return 0;
> + }
> +
> + if (!smc->dr_cpu_enabled) {
> + /*
> + * This is a cold plugged CPU but the machine doesn't support
> + * DR. So skip the hotplug path ensuring that the CPU is brought
> + * up online with out an associated DR connector.
> + */
> + return 0;
> + }
> +
> + g_assert(drc);
> +
> + /*
> + * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> + * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> + */
> + if (dev->hotplugged) {
> + fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
> + }
> +
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + drck->attach(drc, core->dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> + if (local_err) {
> + g_free(fdt);
> + error_propagate(core->errp, local_err);
> + return 1;
> + }
> +
> + /*
> + * We send hotplug notification interrupt to the guest only in case
> + * of hotplugged CPUs.
> + */
> + if (dev->hotplugged) {
> + spapr_hotplug_req_add_by_index(drc);
> + } else {
> + /*
> + * HACK to support removal of hotplugged CPU after VM migration:
> + *
> + * Since we want to be able to hot-remove those coldplugged CPUs
> + * started at boot time using -device option at the target VM, we set
> + * the right allocation_state and isolation_state for them, which for
> + * the hotplugged CPUs would be set via RTAS calls done from the
> + * guest during hotplug.
> + *
> + * This allows the coldplugged CPUs started using -device option to
> + * have the right isolation and allocation states as expected by the
> + * CPU hot removal code.
> + *
> + * This hack will be removed once we have DRC states migrated as part
> + * of VM migration.
> + */
> + drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> + drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
I'm not fully understanding why this is a hack. Aren't those the
right allocation and isolation states for a cpu that was present at
boot?
> + }
> + return 0;
> +}
> +
> +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> + sPAPRCoreState core;
> +
> + core.dev = dev;
> + core.errp = errp;
> + object_child_foreach(OBJECT(dev), spapr_core_attach, &core);
> +}
> +
> static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> int node;
> @@ -2262,6 +2401,34 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> }
>
> spapr_memory_plug(hotplug_dev, dev, node, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> + CPUState *cs = CPU(dev);
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + int i;
> +
> + /* Set NUMA node for the added CPUs */
> + for (i = 0; i < nb_numa_nodes; i++) {
> + if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
> + cs->numa_node = i;
> + break;
> + }
> + }
> +
> + if (!smc->dr_cpu_enabled) {
> + if (dev->hotplugged) {
> + error_setg(errp, "CPU hotplug not supported for this machine");
> + cpu_remove_sync(cs);
> + return;
> + } else {
> + spapr_cpu_init(ms, cpu);
You could just continue onto the code below, yes? the cpu_reset()
would be unnecessary but harmless IIUC.
> + return;
> + }
> + }
> +
> + spapr_cpu_init(ms, cpu);
> + spapr_cpu_reset(cpu);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> + spapr_core_plug(hotplug_dev, dev, errp);
So, I see that there are branches here for both individual vcpu
objects and for cpu core objects. I'm assuming it's only intended
that the user add core objects, and the vcpu path is for the vcpus
constructed by the core object. Is that right?
Does anything enforce that the user can't directly device_add a vcpu
object?
> }
> }
>
> @@ -2276,7 +2443,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> DeviceState *dev)
> {
> - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> return HOTPLUG_HANDLER(machine);
> }
> return NULL;
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 744ea62..1063036 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -436,6 +436,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> case SPAPR_DR_CONNECTOR_TYPE_LMB:
> hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
> break;
> + case SPAPR_DR_CONNECTOR_TYPE_CPU:
> + hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
> + break;
> default:
> /* we shouldn't be signaling hotplug events for resources
> * that don't support them
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 34b12a3..7baa862 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -33,6 +33,7 @@
>
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/spapr_vio.h"
> +#include "hw/ppc/ppc.h"
> #include "qapi-event.h"
> #include "hw/boards.h"
>
> @@ -159,6 +160,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
> rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> }
>
> +/*
> + * Set the timebase offset of the CPU to that of first CPU.
> + * This helps hotplugged CPU to have the correct timebase offset.
> + */
> +static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> +{
> + PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> +
> + cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> +}
> +
> +static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> +{
> + PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> +
> + if (!pcc->interrupts_big_endian(fcpu)) {
> + cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> + }
> +}
> +
> static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> uint32_t token, uint32_t nargs,
> target_ulong args,
> @@ -195,6 +217,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> env->nip = start;
> env->gpr[3] = r3;
> cs->halted = 0;
> + spapr_cpu_set_endianness(cpu);
> + spapr_cpu_update_tb_offset(cpu);
>
> qemu_cpu_kick(cs);
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 739f9ba..68d51d6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -83,6 +83,11 @@ struct sPAPRMachineState {
> MemoryHotplugState hotplug_memory;
> };
>
> +typedef struct sPAPRCoreState {
> + DeviceState *dev;
> + Error **errp;
> +} sPAPRCoreState;
> +
> #define H_SUCCESS 0
> #define H_BUSY 1 /* Hardware busy -- retry later */
> #define H_CLOSED 2 /* Resource closed */
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index d5ae53e..651dd41 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -30,6 +30,9 @@
> #include "qemu/error-report.h"
> #include "qapi/visitor.h"
> #include "hw/qdev-properties.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "sysemu/sysemu.h"
> +#endif
>
> //#define PPC_DUMP_CPU
> //#define PPC_DEBUG_SPR
> @@ -8933,6 +8936,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
> }
>
> #if !defined(CONFIG_USER_ONLY)
> + if (cs->cpu_index >= max_cpus) {
> + error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
> + return;
> + }
> +
> cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
> + (cs->cpu_index % smp_threads);
> #endif
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support
2016-01-12 5:58 ` David Gibson
@ 2016-01-13 3:55 ` Bharata B Rao
0 siblings, 0 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-13 3:55 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
On Tue, Jan 12, 2016 at 04:58:44PM +1100, David Gibson wrote:
> On Fri, Jan 08, 2016 at 12:25:18PM +0530, Bharata B Rao wrote:
> > <snip>
> > +static int spapr_core_attach(Object *obj, void *opaque)
> > +{
> > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> > + sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine());
> > + sPAPRCoreState *core = opaque;
> > + DeviceState *dev = DEVICE(obj);
> > + CPUState *cs = CPU(dev);
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > + int id = ppc_get_vcpu_dt_id(cpu);
> > + sPAPRDRConnector *drc =
> > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> > + sPAPRDRConnectorClass *drck;
> > + int smt = kvmppc_smt_threads();
> > + Error *local_err = NULL;
> > + void *fdt = NULL;
> > + int fdt_offset = 0;
> > +
> > + /*
> > + * Only main SMT thread (thread 0) will continue and signal the
> > + * hotplug event to the guest. Other threads of the core will
> > + * return from here.
> > + */
> > + if ((id % smt) != 0) {
> > + return 0;
> > + }
> > +
> > + if (!smc->dr_cpu_enabled) {
> > + /*
> > + * This is a cold plugged CPU but the machine doesn't support
> > + * DR. So skip the hotplug path ensuring that the CPU is brought
> > + * up online with out an associated DR connector.
> > + */
> > + return 0;
> > + }
> > +
> > + g_assert(drc);
> > +
> > + /*
> > + * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> > + * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> > + */
> > + if (dev->hotplugged) {
> > + fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
> > + }
> > +
> > + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > + drck->attach(drc, core->dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> > + if (local_err) {
> > + g_free(fdt);
> > + error_propagate(core->errp, local_err);
> > + return 1;
> > + }
> > +
> > + /*
> > + * We send hotplug notification interrupt to the guest only in case
> > + * of hotplugged CPUs.
> > + */
> > + if (dev->hotplugged) {
> > + spapr_hotplug_req_add_by_index(drc);
> > + } else {
> > + /*
> > + * HACK to support removal of hotplugged CPU after VM migration:
> > + *
> > + * Since we want to be able to hot-remove those coldplugged CPUs
> > + * started at boot time using -device option at the target VM, we set
> > + * the right allocation_state and isolation_state for them, which for
> > + * the hotplugged CPUs would be set via RTAS calls done from the
> > + * guest during hotplug.
> > + *
> > + * This allows the coldplugged CPUs started using -device option to
> > + * have the right isolation and allocation states as expected by the
> > + * CPU hot removal code.
> > + *
> > + * This hack will be removed once we have DRC states migrated as part
> > + * of VM migration.
> > + */
> > + drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > + drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
>
> I'm not fully understanding why this is a hack. Aren't those the
> right allocation and isolation states for a cpu that was present at
> boot?
Those comments are already old, will remove them. I remember Michael Roth
confirming that setting the initial DRC states like this for cold plugged
CPUs should be alright.
>
> > + }
> > + return 0;
> > +}
> > +
> > +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > + Error **errp)
> > +{
> > + sPAPRCoreState core;
> > +
> > + core.dev = dev;
> > + core.errp = errp;
> > + object_child_foreach(OBJECT(dev), spapr_core_attach, &core);
> > +}
> > +
> > static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > int node;
> > @@ -2262,6 +2401,34 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > }
> >
> > spapr_memory_plug(hotplug_dev, dev, node, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> > + CPUState *cs = CPU(dev);
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > + int i;
> > +
> > + /* Set NUMA node for the added CPUs */
> > + for (i = 0; i < nb_numa_nodes; i++) {
> > + if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
> > + cs->numa_node = i;
> > + break;
> > + }
> > + }
> > +
> > + if (!smc->dr_cpu_enabled) {
> > + if (dev->hotplugged) {
> > + error_setg(errp, "CPU hotplug not supported for this machine");
> > + cpu_remove_sync(cs);
> > + return;
> > + } else {
> > + spapr_cpu_init(ms, cpu);
>
> You could just continue onto the code below, yes? the cpu_reset()
> would be unnecessary but harmless IIUC.
Will do that.
>
> > + return;
> > + }
> > + }
> > +
> > + spapr_cpu_init(ms, cpu);
> > + spapr_cpu_reset(cpu);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> > + spapr_core_plug(hotplug_dev, dev, errp);
>
> So, I see that there are branches here for both individual vcpu
> objects and for cpu core objects. I'm assuming it's only intended
> that the user add core objects, and the vcpu path is for the vcpus
> constructed by the core object. Is that right?
That's correct.
>
> Does anything enforce that the user can't directly device_add a vcpu
> object?
CPU objects (like host-powerpc64-cpu or POWER8-powerpc64-cpu etc) will not
be exposed to device_add command since they don't have
cannot_instantiate_with_device_add_yet memer of their DeviceClass set to
false.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support Bharata B Rao
2016-01-12 5:58 ` David Gibson
@ 2016-01-12 23:58 ` Alexey Kardashevskiy
2016-01-13 4:01 ` Bharata B Rao
1 sibling, 1 reply; 39+ messages in thread
From: Alexey Kardashevskiy @ 2016-01-12 23:58 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel
Cc: ehabkost, mdroth, agraf, pbonzini, qemu-ppc, tyreld, nfont,
imammedo, afaerber, david
On 01/08/2016 05:55 PM, Bharata B Rao wrote:
> Support CPU hotplug via device-add command like this:
>
> (qemu) device_add powerpc64-cpu-core,id=core2
>
> In response to device_add, CPU core device will be created. CPU core
> device creates and realizes CPU thread devices. If the machine type
> supports CPU hotplug, boot-time CPUs are created as CPU core devices
> otherwise they continue to be created as individual CPU devices.
>
> Set up device tree entries for the hotplugged CPU core and use the
> exising EPOW event infrastructure to send CPU hotplug notification to
> the guest.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 183 ++++++++++++++++++++++++++++++++++++++++++--
> hw/ppc/spapr_events.c | 3 +
> hw/ppc/spapr_rtas.c | 24 ++++++
> include/hw/ppc/spapr.h | 5 ++
> target-ppc/translate_init.c | 8 ++
> 5 files changed, 216 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a3ce1db..c2af9ca 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -63,6 +63,7 @@
>
> #include "hw/compat.h"
> #include "qemu-common.h"
> +#include "hw/ppc/cpu-core.h"
>
> #include <libfdt.h>
>
> @@ -600,6 +601,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> size_t page_sizes_prop_size;
> uint32_t vcpus_per_socket = smp_threads * smp_cores;
> uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> + sPAPRDRConnector *drc;
> + sPAPRDRConnectorClass *drck;
> + int drc_index;
> +
> + if (smc->dr_cpu_enabled) {
> + drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> + g_assert(drc);
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + drc_index = drck->get_index(drc);
> + _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> + }
>
> /* Note: we keep CI large pages off for now because a 64K capable guest
> * provisioned with large pages might otherwise try to map a qemu
> @@ -1743,6 +1756,8 @@ static void ppc_spapr_init(MachineState *machine)
> char *filename;
> int smt = kvmppc_smt_threads();
> int smp_max_cores = max_cpus/smp_threads;
> + int spapr_smp_cores = smp_cpus/smp_threads;
> + Object *core;
>
> msi_supported = true;
>
> @@ -1822,13 +1837,22 @@ static void ppc_spapr_init(MachineState *machine)
> if (machine->cpu_model == NULL) {
> machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> }
> - for (i = 0; i < smp_cpus; i++) {
> - cpu = cpu_ppc_init(machine->cpu_model);
> - if (cpu == NULL) {
> - fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> - exit(1);
> +
> + if (smc->dr_cpu_enabled) {
> + for (i = 0; i < spapr_smp_cores; i++) {
> + core = object_new(TYPE_POWERPC_CPU_CORE);
> + object_property_set_bool(core, true, "realized", &error_abort);
> + }
> + } else {
> + for (i = 0; i < smp_cpus; i++) {
> + cpu = cpu_ppc_init(machine->cpu_model);
> + if (cpu == NULL) {
> + fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> + exit(1);
> + }
> + object_property_set_bool(OBJECT(cpu), true, "realized",
> + &error_abort);
> }
> - spapr_cpu_init(spapr, cpu);
> }
>
> if (kvm_enabled()) {
> @@ -2222,10 +2246,125 @@ out:
> error_propagate(errp, local_err);
> }
>
> +static void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> + int *fdt_offset,
> + sPAPRMachineState *spapr)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + DeviceClass *dc = DEVICE_GET_CLASS(cs);
> + int id = ppc_get_vcpu_dt_id(cpu);
> + void *fdt;
> + int offset, fdt_size;
> + char *nodename;
> +
> + fdt = create_device_tree(&fdt_size);
> + nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
> + offset = fdt_add_subnode(fdt, 0, nodename);
> +
> + spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> + g_free(nodename);
> +
> + *fdt_offset = offset;
> + return fdt;
> +}
> +
> +static int spapr_core_attach(Object *obj, void *opaque)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> + sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine());
> + sPAPRCoreState *core = opaque;
> + DeviceState *dev = DEVICE(obj);
> + CPUState *cs = CPU(dev);
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + int id = ppc_get_vcpu_dt_id(cpu);
> + sPAPRDRConnector *drc =
> + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> + sPAPRDRConnectorClass *drck;
> + int smt = kvmppc_smt_threads();
> + Error *local_err = NULL;
> + void *fdt = NULL;
> + int fdt_offset = 0;
> +
> + /*
> + * Only main SMT thread (thread 0) will continue and signal the
> + * hotplug event to the guest. Other threads of the core will
> + * return from here.
> + */
> + if ((id % smt) != 0) {
> + return 0;
> + }
> +
> + if (!smc->dr_cpu_enabled) {
> + /*
> + * This is a cold plugged CPU but the machine doesn't support
> + * DR. So skip the hotplug path ensuring that the CPU is brought
> + * up online with out an associated DR connector.
> + */
> + return 0;
> + }
> +
> + g_assert(drc);
> +
> + /*
> + * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> + * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> + */
> + if (dev->hotplugged) {
> + fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
> + }
spapr_core_attach() is only called from spapr_core_plug() which is only
called from spapr_machine_device_plug() which is a hotplug handler so the
check for dev->hotplugged seems redundant here and below, no? Or this is
called at the boot time for cold-plug devices?
> +
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + drck->attach(drc, core->dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> + if (local_err) {
> + g_free(fdt);
> + error_propagate(core->errp, local_err);
> + return 1;
> + }
> +
> + /*
> + * We send hotplug notification interrupt to the guest only in case
> + * of hotplugged CPUs.
> + */
> + if (dev->hotplugged) {
> + spapr_hotplug_req_add_by_index(drc);
> + } else {
> + /*
> + * HACK to support removal of hotplugged CPU after VM migration:
> + *
> + * Since we want to be able to hot-remove those coldplugged CPUs
> + * started at boot time using -device option at the target VM, we set
> + * the right allocation_state and isolation_state for them, which for
> + * the hotplugged CPUs would be set via RTAS calls done from the
> + * guest during hotplug.
> + *
> + * This allows the coldplugged CPUs started using -device option to
> + * have the right isolation and allocation states as expected by the
> + * CPU hot removal code.
> + *
> + * This hack will be removed once we have DRC states migrated as part
> + * of VM migration.
> + */
> + drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> + drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> + }
> + return 0;
> +}
> +
> +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> + sPAPRCoreState core;
> +
> + core.dev = dev;
> + core.errp = errp;
> + object_child_foreach(OBJECT(dev), spapr_core_attach, &core);
> +}
> +
> static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> int node;
> @@ -2262,6 +2401,34 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> }
>
> spapr_memory_plug(hotplug_dev, dev, node, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> + CPUState *cs = CPU(dev);
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + int i;
> +
> + /* Set NUMA node for the added CPUs */
> + for (i = 0; i < nb_numa_nodes; i++) {
> + if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
> + cs->numa_node = i;
> + break;
> + }
> + }
> +
> + if (!smc->dr_cpu_enabled) {
> + if (dev->hotplugged) {
> + error_setg(errp, "CPU hotplug not supported for this machine");
> + cpu_remove_sync(cs);
> + return;
> + } else {
> + spapr_cpu_init(ms, cpu);
> + return;
> + }
> + }
> +
> + spapr_cpu_init(ms, cpu);
> + spapr_cpu_reset(cpu);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> + spapr_core_plug(hotplug_dev, dev, errp);
> }
> }
>
> @@ -2276,7 +2443,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> DeviceState *dev)
> {
> - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> return HOTPLUG_HANDLER(machine);
> }
> return NULL;
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 744ea62..1063036 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -436,6 +436,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> case SPAPR_DR_CONNECTOR_TYPE_LMB:
> hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
> break;
> + case SPAPR_DR_CONNECTOR_TYPE_CPU:
> + hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
> + break;
> default:
> /* we shouldn't be signaling hotplug events for resources
> * that don't support them
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 34b12a3..7baa862 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -33,6 +33,7 @@
>
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/spapr_vio.h"
> +#include "hw/ppc/ppc.h"
> #include "qapi-event.h"
> #include "hw/boards.h"
>
> @@ -159,6 +160,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
> rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> }
>
> +/*
> + * Set the timebase offset of the CPU to that of first CPU.
> + * This helps hotplugged CPU to have the correct timebase offset.
> + */
> +static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> +{
> + PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> +
> + cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> +}
> +
> +static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> +{
> + PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> +
> + if (!pcc->interrupts_big_endian(fcpu)) {
> + cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> + }
> +}
> +
> static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> uint32_t token, uint32_t nargs,
> target_ulong args,
> @@ -195,6 +217,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> env->nip = start;
> env->gpr[3] = r3;
> cs->halted = 0;
> + spapr_cpu_set_endianness(cpu);
> + spapr_cpu_update_tb_offset(cpu);
>
> qemu_cpu_kick(cs);
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 739f9ba..68d51d6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -83,6 +83,11 @@ struct sPAPRMachineState {
> MemoryHotplugState hotplug_memory;
> };
>
> +typedef struct sPAPRCoreState {
> + DeviceState *dev;
> + Error **errp;
> +} sPAPRCoreState;
> +
> #define H_SUCCESS 0
> #define H_BUSY 1 /* Hardware busy -- retry later */
> #define H_CLOSED 2 /* Resource closed */
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index d5ae53e..651dd41 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -30,6 +30,9 @@
> #include "qemu/error-report.h"
> #include "qapi/visitor.h"
> #include "hw/qdev-properties.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "sysemu/sysemu.h"
> +#endif
>
> //#define PPC_DUMP_CPU
> //#define PPC_DEBUG_SPR
> @@ -8933,6 +8936,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
> }
>
> #if !defined(CONFIG_USER_ONLY)
> + if (cs->cpu_index >= max_cpus) {
> + error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
> + return;
> + }
> +
> cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
> + (cs->cpu_index % smp_threads);
> #endif
>
--
Alexey
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support
2016-01-12 23:58 ` Alexey Kardashevskiy
@ 2016-01-13 4:01 ` Bharata B Rao
0 siblings, 0 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-13 4:01 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: mdroth, agraf, ehabkost, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, david
On Wed, Jan 13, 2016 at 10:58:06AM +1100, Alexey Kardashevskiy wrote:
> On 01/08/2016 05:55 PM, Bharata B Rao wrote:
> >Support CPU hotplug via device-add command like this:
> >
> >(qemu) device_add powerpc64-cpu-core,id=core2
> >
> >In response to device_add, CPU core device will be created. CPU core
> >device creates and realizes CPU thread devices. If the machine type
> >supports CPU hotplug, boot-time CPUs are created as CPU core devices
> >otherwise they continue to be created as individual CPU devices.
> >
> >Set up device tree entries for the hotplugged CPU core and use the
> >exising EPOW event infrastructure to send CPU hotplug notification to
> >the guest.
> >
> >Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >---
> > hw/ppc/spapr.c | 183 ++++++++++++++++++++++++++++++++++++++++++--
> > hw/ppc/spapr_events.c | 3 +
> > hw/ppc/spapr_rtas.c | 24 ++++++
> > include/hw/ppc/spapr.h | 5 ++
> > target-ppc/translate_init.c | 8 ++
> > 5 files changed, 216 insertions(+), 7 deletions(-)
> >
> >diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >index a3ce1db..c2af9ca 100644
> >--- a/hw/ppc/spapr.c
> >+++ b/hw/ppc/spapr.c
> >@@ -63,6 +63,7 @@
> >
> > #include "hw/compat.h"
> > #include "qemu-common.h"
> >+#include "hw/ppc/cpu-core.h"
> >
> > #include <libfdt.h>
> >
> >@@ -600,6 +601,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> > size_t page_sizes_prop_size;
> > uint32_t vcpus_per_socket = smp_threads * smp_cores;
> > uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> >+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> >+ sPAPRDRConnector *drc;
> >+ sPAPRDRConnectorClass *drck;
> >+ int drc_index;
> >+
> >+ if (smc->dr_cpu_enabled) {
> >+ drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> >+ g_assert(drc);
> >+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >+ drc_index = drck->get_index(drc);
> >+ _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> >+ }
> >
> > /* Note: we keep CI large pages off for now because a 64K capable guest
> > * provisioned with large pages might otherwise try to map a qemu
> >@@ -1743,6 +1756,8 @@ static void ppc_spapr_init(MachineState *machine)
> > char *filename;
> > int smt = kvmppc_smt_threads();
> > int smp_max_cores = max_cpus/smp_threads;
> >+ int spapr_smp_cores = smp_cpus/smp_threads;
> >+ Object *core;
> >
> > msi_supported = true;
> >
> >@@ -1822,13 +1837,22 @@ static void ppc_spapr_init(MachineState *machine)
> > if (machine->cpu_model == NULL) {
> > machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > }
> >- for (i = 0; i < smp_cpus; i++) {
> >- cpu = cpu_ppc_init(machine->cpu_model);
> >- if (cpu == NULL) {
> >- fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> >- exit(1);
> >+
> >+ if (smc->dr_cpu_enabled) {
> >+ for (i = 0; i < spapr_smp_cores; i++) {
> >+ core = object_new(TYPE_POWERPC_CPU_CORE);
> >+ object_property_set_bool(core, true, "realized", &error_abort);
> >+ }
> >+ } else {
> >+ for (i = 0; i < smp_cpus; i++) {
> >+ cpu = cpu_ppc_init(machine->cpu_model);
> >+ if (cpu == NULL) {
> >+ fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> >+ exit(1);
> >+ }
> >+ object_property_set_bool(OBJECT(cpu), true, "realized",
> >+ &error_abort);
> > }
> >- spapr_cpu_init(spapr, cpu);
> > }
> >
> > if (kvm_enabled()) {
> >@@ -2222,10 +2246,125 @@ out:
> > error_propagate(errp, local_err);
> > }
> >
> >+static void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> >+ int *fdt_offset,
> >+ sPAPRMachineState *spapr)
> >+{
> >+ PowerPCCPU *cpu = POWERPC_CPU(cs);
> >+ DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >+ int id = ppc_get_vcpu_dt_id(cpu);
> >+ void *fdt;
> >+ int offset, fdt_size;
> >+ char *nodename;
> >+
> >+ fdt = create_device_tree(&fdt_size);
> >+ nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
> >+ offset = fdt_add_subnode(fdt, 0, nodename);
> >+
> >+ spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> >+ g_free(nodename);
> >+
> >+ *fdt_offset = offset;
> >+ return fdt;
> >+}
> >+
> >+static int spapr_core_attach(Object *obj, void *opaque)
> >+{
> >+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> >+ sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine());
> >+ sPAPRCoreState *core = opaque;
> >+ DeviceState *dev = DEVICE(obj);
> >+ CPUState *cs = CPU(dev);
> >+ PowerPCCPU *cpu = POWERPC_CPU(cs);
> >+ int id = ppc_get_vcpu_dt_id(cpu);
> >+ sPAPRDRConnector *drc =
> >+ spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> >+ sPAPRDRConnectorClass *drck;
> >+ int smt = kvmppc_smt_threads();
> >+ Error *local_err = NULL;
> >+ void *fdt = NULL;
> >+ int fdt_offset = 0;
> >+
> >+ /*
> >+ * Only main SMT thread (thread 0) will continue and signal the
> >+ * hotplug event to the guest. Other threads of the core will
> >+ * return from here.
> >+ */
> >+ if ((id % smt) != 0) {
> >+ return 0;
> >+ }
> >+
> >+ if (!smc->dr_cpu_enabled) {
> >+ /*
> >+ * This is a cold plugged CPU but the machine doesn't support
> >+ * DR. So skip the hotplug path ensuring that the CPU is brought
> >+ * up online with out an associated DR connector.
> >+ */
> >+ return 0;
> >+ }
> >+
> >+ g_assert(drc);
> >+
> >+ /*
> >+ * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> >+ * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> >+ */
> >+ if (dev->hotplugged) {
> >+ fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
> >+ }
>
>
> spapr_core_attach() is only called from spapr_core_plug() which is only
> called from spapr_machine_device_plug() which is a hotplug handler so the
> check for dev->hotplugged seems redundant here and below, no? Or this is
> called at the boot time for cold-plug devices?
->plug() handler will be called for cold plugged CPUs too. For those CPUs
specified using -smp option and for those specified using -device option,
->plug() handler will be called because I am initializing these boot time
CPUs as CPU core devices.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support
2016-01-08 6:55 [Qemu-devel] [PATCH v6 00/11] sPAPR CPU hotplug Bharata B Rao
` (9 preceding siblings ...)
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support Bharata B Rao
@ 2016-01-08 6:55 ` Bharata B Rao
2016-01-12 6:06 ` David Gibson
10 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-08 6:55 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, aik, Bharata B Rao, mdroth, agraf, pbonzini, qemu-ppc,
tyreld, nfont, imammedo, afaerber, david
Remove the CPU core device by removing the underlying CPU thread devices.
Support hot removal of CPU for sPAPR guests by sending the hot unplug
notification to the guest via EPOW interrupt. Release the vCPU object
after CPU hot unplug so that vCPU fd can be parked and reused.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/ppc/spapr.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr.h | 6 +++
2 files changed, 119 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c2af9ca..43cb726 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -93,6 +93,9 @@
#define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
+static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list
+ = QLIST_HEAD_INITIALIZER(cpu_unplug_list);
+
static XICSState *try_create_xics(const char *type, int nr_servers,
int nr_irqs, Error **errp)
{
@@ -2432,11 +2435,121 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
}
}
+static void spapr_cpu_destroy(PowerPCCPU *cpu)
+{
+ sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+
+ xics_cpu_destroy(spapr->icp, cpu);
+ qemu_unregister_reset(spapr_cpu_reset, cpu);
+}
+
+static void spapr_cpu_core_cleanup(void)
+{
+ sPAPRCPUUnplug *unplug, *next;
+ Object *cpu;
+
+ QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) {
+ cpu = unplug->cpu;
+ object_unparent(cpu);
+ QLIST_REMOVE(unplug, node);
+ g_free(unplug);
+ }
+}
+
+static void spapr_add_cpu_to_unplug_list(Object *cpu)
+{
+ sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
+
+ unplug->cpu = cpu;
+ QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node);
+}
+
+static int spapr_cpu_release(Object *obj, void *opaque)
+{
+ DeviceState *dev = DEVICE(obj);
+ CPUState *cs = CPU(dev);
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+ spapr_cpu_destroy(cpu);
+ cpu_remove_sync(cs);
+
+ /*
+ * We are still walking the core object's children list, and
+ * hence can't cleanup this CPU thread object just yet. Put
+ * it on a list for later removal.
+ */
+ spapr_add_cpu_to_unplug_list(obj);
+ return 0;
+}
+
+static void spapr_core_release(DeviceState *dev, void *opaque)
+{
+ object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque);
+ spapr_cpu_core_cleanup();
+ object_unparent(OBJECT(dev));
+}
+
+static int spapr_core_detach(Object *obj, void *opaque)
+{
+ sPAPRCoreState *core = opaque;
+ DeviceState *dev = DEVICE(obj);
+ CPUState *cs = CPU(dev);
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ int id = ppc_get_vcpu_dt_id(cpu);
+ int smt = kvmppc_smt_threads();
+ sPAPRDRConnector *drc =
+ spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
+ sPAPRDRConnectorClass *drck;
+ Error *local_err = NULL;
+
+ /*
+ * SMT threads return from here, only main thread (thread 0) will
+ * continue and signal hot unplug event to the guest.
+ */
+ if ((id % smt) != 0) {
+ return 0;
+ }
+ g_assert(drc);
+
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ drck->detach(drc, core->dev, spapr_core_release, NULL, &local_err);
+ if (local_err) {
+ error_propagate(core->errp, local_err);
+ return 1;
+ }
+
+ /*
+ * In addition to hotplugged CPUs, send the hot-unplug notification
+ * interrupt to the guest for coldplugged CPUs started via -device
+ * option too.
+ */
+ spapr_hotplug_req_remove_by_index(drc);
+ return 0;
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+ sPAPRCoreState core;
+
+ core.dev = dev;
+ core.errp = errp;
+ object_child_foreach(OBJECT(dev), spapr_core_detach, &core);
+}
+
static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
error_setg(errp, "Memory hot unplug not supported by sPAPR");
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
+ if (!smc->dr_cpu_enabled) {
+ error_setg(errp, "CPU hot unplug not supported on this machine");
+ return;
+ }
+ spapr_core_unplug(hotplug_dev, dev, errp);
}
}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 68d51d6..4d89016 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -88,6 +88,12 @@ typedef struct sPAPRCoreState {
Error **errp;
} sPAPRCoreState;
+/* List to store unplugged CPU objects for cleanup during unplug */
+typedef struct sPAPRCPUUnplug {
+ Object *cpu;
+ QLIST_ENTRY(sPAPRCPUUnplug) node;
+} sPAPRCPUUnplug;
+
#define H_SUCCESS 0
#define H_BUSY 1 /* Hardware busy -- retry later */
#define H_CLOSED 2 /* Resource closed */
--
2.1.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support
2016-01-08 6:55 ` [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support Bharata B Rao
@ 2016-01-12 6:06 ` David Gibson
2016-01-13 4:10 ` Bharata B Rao
0 siblings, 1 reply; 39+ messages in thread
From: David Gibson @ 2016-01-12 6:06 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 6292 bytes --]
On Fri, Jan 08, 2016 at 12:25:19PM +0530, Bharata B Rao wrote:
> Remove the CPU core device by removing the underlying CPU thread devices.
> Support hot removal of CPU for sPAPR guests by sending the hot unplug
> notification to the guest via EPOW interrupt. Release the vCPU object
> after CPU hot unplug so that vCPU fd can be parked and reused.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 6 +++
> 2 files changed, 119 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c2af9ca..43cb726 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -93,6 +93,9 @@
>
> #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
>
> +static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list
> + = QLIST_HEAD_INITIALIZER(cpu_unplug_list);
> +
> static XICSState *try_create_xics(const char *type, int nr_servers,
> int nr_irqs, Error **errp)
> {
> @@ -2432,11 +2435,121 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> }
> }
>
> +static void spapr_cpu_destroy(PowerPCCPU *cpu)
> +{
> + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +
> + xics_cpu_destroy(spapr->icp, cpu);
> + qemu_unregister_reset(spapr_cpu_reset, cpu);
> +}
> +
> +static void spapr_cpu_core_cleanup(void)
> +{
> + sPAPRCPUUnplug *unplug, *next;
> + Object *cpu;
> +
> + QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) {
> + cpu = unplug->cpu;
> + object_unparent(cpu);
> + QLIST_REMOVE(unplug, node);
> + g_free(unplug);
> + }
> +}
> +
> +static void spapr_add_cpu_to_unplug_list(Object *cpu)
> +{
> + sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
> +
> + unplug->cpu = cpu;
> + QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node);
> +}
> +
> +static int spapr_cpu_release(Object *obj, void *opaque)
> +{
> + DeviceState *dev = DEVICE(obj);
> + CPUState *cs = CPU(dev);
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> + spapr_cpu_destroy(cpu);
> + cpu_remove_sync(cs);
> +
> + /*
> + * We are still walking the core object's children list, and
> + * hence can't cleanup this CPU thread object just yet. Put
> + * it on a list for later removal.
> + */
> + spapr_add_cpu_to_unplug_list(obj);
> + return 0;
> +}
> +
> +static void spapr_core_release(DeviceState *dev, void *opaque)
> +{
> + object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque);
> + spapr_cpu_core_cleanup();
> + object_unparent(OBJECT(dev));
I'd prefer to see the unplug_list as a local to this function (passed
to spapr_cpu_release via opaque) rather than using a new global.
> +}
> +
> +static int spapr_core_detach(Object *obj, void *opaque)
> +{
> + sPAPRCoreState *core = opaque;
> + DeviceState *dev = DEVICE(obj);
> + CPUState *cs = CPU(dev);
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + int id = ppc_get_vcpu_dt_id(cpu);
> + int smt = kvmppc_smt_threads();
> + sPAPRDRConnector *drc =
> + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> + sPAPRDRConnectorClass *drck;
> + Error *local_err = NULL;
> +
> + /*
> + * SMT threads return from here, only main thread (thread 0) will
> + * continue and signal hot unplug event to the guest.
> + */
This seems silly. spapr_core_unplug() iterates through all the
children only to have all of them except thread 0 ignore the call..
Can't you just grab the first thread and do this logic directly from
spapr_core_unplug()?
Same question for the plug side, though I didn't think of it when I
was looking at that.
> + if ((id % smt) != 0) {
> + return 0;
> + }
> + g_assert(drc);
> +
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + drck->detach(drc, core->dev, spapr_core_release, NULL, &local_err);
> + if (local_err) {
> + error_propagate(core->errp, local_err);
> + return 1;
> + }
> +
> + /*
> + * In addition to hotplugged CPUs, send the hot-unplug notification
> + * interrupt to the guest for coldplugged CPUs started via -device
> + * option too.
> + */
> + spapr_hotplug_req_remove_by_index(drc);
> + return 0;
> +}
> +
> +static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> + sPAPRCoreState core;
> +
> + core.dev = dev;
> + core.errp = errp;
> + object_child_foreach(OBJECT(dev), spapr_core_detach, &core);
> +}
> +
> static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> error_setg(errp, "Memory hot unplug not supported by sPAPR");
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> + if (!smc->dr_cpu_enabled) {
> + error_setg(errp, "CPU hot unplug not supported on this machine");
> + return;
> + }
> + spapr_core_unplug(hotplug_dev, dev, errp);
> }
> }
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 68d51d6..4d89016 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -88,6 +88,12 @@ typedef struct sPAPRCoreState {
> Error **errp;
> } sPAPRCoreState;
>
> +/* List to store unplugged CPU objects for cleanup during unplug */
> +typedef struct sPAPRCPUUnplug {
> + Object *cpu;
> + QLIST_ENTRY(sPAPRCPUUnplug) node;
> +} sPAPRCPUUnplug;
> +
> #define H_SUCCESS 0
> #define H_BUSY 1 /* Hardware busy -- retry later */
> #define H_CLOSED 2 /* Resource closed */
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support
2016-01-12 6:06 ` David Gibson
@ 2016-01-13 4:10 ` Bharata B Rao
2016-01-13 4:57 ` David Gibson
0 siblings, 1 reply; 39+ messages in thread
From: Bharata B Rao @ 2016-01-13 4:10 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
On Tue, Jan 12, 2016 at 05:06:34PM +1100, David Gibson wrote:
> On Fri, Jan 08, 2016 at 12:25:19PM +0530, Bharata B Rao wrote:
> > Remove the CPU core device by removing the underlying CPU thread devices.
> > Support hot removal of CPU for sPAPR guests by sending the hot unplug
> > notification to the guest via EPOW interrupt. Release the vCPU object
> > after CPU hot unplug so that vCPU fd can be parked and reused.
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > hw/ppc/spapr.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr.h | 6 +++
> > 2 files changed, 119 insertions(+)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c2af9ca..43cb726 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -93,6 +93,9 @@
> >
> > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
> >
> > +static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list
> > + = QLIST_HEAD_INITIALIZER(cpu_unplug_list);
> > +
> > static XICSState *try_create_xics(const char *type, int nr_servers,
> > int nr_irqs, Error **errp)
> > {
> > @@ -2432,11 +2435,121 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > }
> > }
> >
> > +static void spapr_cpu_destroy(PowerPCCPU *cpu)
> > +{
> > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +
> > + xics_cpu_destroy(spapr->icp, cpu);
> > + qemu_unregister_reset(spapr_cpu_reset, cpu);
> > +}
> > +
> > +static void spapr_cpu_core_cleanup(void)
> > +{
> > + sPAPRCPUUnplug *unplug, *next;
> > + Object *cpu;
> > +
> > + QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) {
> > + cpu = unplug->cpu;
> > + object_unparent(cpu);
> > + QLIST_REMOVE(unplug, node);
> > + g_free(unplug);
> > + }
> > +}
> > +
> > +static void spapr_add_cpu_to_unplug_list(Object *cpu)
> > +{
> > + sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
> > +
> > + unplug->cpu = cpu;
> > + QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node);
> > +}
> > +
> > +static int spapr_cpu_release(Object *obj, void *opaque)
> > +{
> > + DeviceState *dev = DEVICE(obj);
> > + CPUState *cs = CPU(dev);
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > + spapr_cpu_destroy(cpu);
> > + cpu_remove_sync(cs);
> > +
> > + /*
> > + * We are still walking the core object's children list, and
> > + * hence can't cleanup this CPU thread object just yet. Put
> > + * it on a list for later removal.
> > + */
> > + spapr_add_cpu_to_unplug_list(obj);
> > + return 0;
> > +}
> > +
> > +static void spapr_core_release(DeviceState *dev, void *opaque)
> > +{
> > + object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque);
> > + spapr_cpu_core_cleanup();
> > + object_unparent(OBJECT(dev));
>
> I'd prefer to see the unplug_list as a local to this function (passed
> to spapr_cpu_release via opaque) rather than using a new global.
Will try that in the next iteration.
>
> > +}
> > +
> > +static int spapr_core_detach(Object *obj, void *opaque)
> > +{
> > + sPAPRCoreState *core = opaque;
> > + DeviceState *dev = DEVICE(obj);
> > + CPUState *cs = CPU(dev);
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > + int id = ppc_get_vcpu_dt_id(cpu);
> > + int smt = kvmppc_smt_threads();
> > + sPAPRDRConnector *drc =
> > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> > + sPAPRDRConnectorClass *drck;
> > + Error *local_err = NULL;
> > +
> > + /*
> > + * SMT threads return from here, only main thread (thread 0) will
> > + * continue and signal hot unplug event to the guest.
> > + */
>
> This seems silly. spapr_core_unplug() iterates through all the
> children only to have all of them except thread 0 ignore the call..
>
> Can't you just grab the first thread and do this logic directly from
> spapr_core_unplug()?
If I purely rely on the children list of the core object like I am doing
here, I don't see how to grab the first thread object from the list w/o
doing what I am doing here. However I can store the first thread object
in the core object during the core object's instance_init and use it here.
Will give it a try in the next iteration.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support
2016-01-13 4:10 ` Bharata B Rao
@ 2016-01-13 4:57 ` David Gibson
2016-01-13 7:04 ` Bharata B Rao
0 siblings, 1 reply; 39+ messages in thread
From: David Gibson @ 2016-01-13 4:57 UTC (permalink / raw)
To: Bharata B Rao
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]
On Wed, Jan 13, 2016 at 09:40:54AM +0530, Bharata B Rao wrote:
> On Tue, Jan 12, 2016 at 05:06:34PM +1100, David Gibson wrote:
> > On Fri, Jan 08, 2016 at 12:25:19PM +0530, Bharata B Rao wrote:
> > > Remove the CPU core device by removing the underlying CPU thread devices.
> > > Support hot removal of CPU for sPAPR guests by sending the hot unplug
> > > notification to the guest via EPOW interrupt. Release the vCPU object
> > > after CPU hot unplug so that vCPU fd can be parked and reused.
> > >
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > > hw/ppc/spapr.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > include/hw/ppc/spapr.h | 6 +++
> > > 2 files changed, 119 insertions(+)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index c2af9ca..43cb726 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -93,6 +93,9 @@
> > >
> > > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
> > >
> > > +static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list
> > > + = QLIST_HEAD_INITIALIZER(cpu_unplug_list);
> > > +
> > > static XICSState *try_create_xics(const char *type, int nr_servers,
> > > int nr_irqs, Error **errp)
> > > {
> > > @@ -2432,11 +2435,121 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > }
> > > }
> > >
> > > +static void spapr_cpu_destroy(PowerPCCPU *cpu)
> > > +{
> > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +
> > > + xics_cpu_destroy(spapr->icp, cpu);
> > > + qemu_unregister_reset(spapr_cpu_reset, cpu);
> > > +}
> > > +
> > > +static void spapr_cpu_core_cleanup(void)
> > > +{
> > > + sPAPRCPUUnplug *unplug, *next;
> > > + Object *cpu;
> > > +
> > > + QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) {
> > > + cpu = unplug->cpu;
> > > + object_unparent(cpu);
> > > + QLIST_REMOVE(unplug, node);
> > > + g_free(unplug);
> > > + }
> > > +}
> > > +
> > > +static void spapr_add_cpu_to_unplug_list(Object *cpu)
> > > +{
> > > + sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
> > > +
> > > + unplug->cpu = cpu;
> > > + QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node);
> > > +}
> > > +
> > > +static int spapr_cpu_release(Object *obj, void *opaque)
> > > +{
> > > + DeviceState *dev = DEVICE(obj);
> > > + CPUState *cs = CPU(dev);
> > > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > + spapr_cpu_destroy(cpu);
> > > + cpu_remove_sync(cs);
> > > +
> > > + /*
> > > + * We are still walking the core object's children list, and
> > > + * hence can't cleanup this CPU thread object just yet. Put
> > > + * it on a list for later removal.
> > > + */
> > > + spapr_add_cpu_to_unplug_list(obj);
> > > + return 0;
> > > +}
> > > +
> > > +static void spapr_core_release(DeviceState *dev, void *opaque)
> > > +{
> > > + object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque);
> > > + spapr_cpu_core_cleanup();
> > > + object_unparent(OBJECT(dev));
> >
> > I'd prefer to see the unplug_list as a local to this function (passed
> > to spapr_cpu_release via opaque) rather than using a new global.
>
> Will try that in the next iteration.
>
> >
> > > +}
> > > +
> > > +static int spapr_core_detach(Object *obj, void *opaque)
> > > +{
> > > + sPAPRCoreState *core = opaque;
> > > + DeviceState *dev = DEVICE(obj);
> > > + CPUState *cs = CPU(dev);
> > > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > + int id = ppc_get_vcpu_dt_id(cpu);
> > > + int smt = kvmppc_smt_threads();
> > > + sPAPRDRConnector *drc =
> > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> > > + sPAPRDRConnectorClass *drck;
> > > + Error *local_err = NULL;
> > > +
> > > + /*
> > > + * SMT threads return from here, only main thread (thread 0) will
> > > + * continue and signal hot unplug event to the guest.
> > > + */
> >
> > This seems silly. spapr_core_unplug() iterates through all the
> > children only to have all of them except thread 0 ignore the call..
> >
> > Can't you just grab the first thread and do this logic directly from
> > spapr_core_unplug()?
>
> If I purely rely on the children list of the core object like I am doing
> here, I don't see how to grab the first thread object from the list w/o
> doing what I am doing here. However I can store the first thread object
> in the core object during the core object's instance_init and use it here.
> Will give it a try in the next iteration.
It should be accessible by name as "thread[0]" no?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support
2016-01-13 4:57 ` David Gibson
@ 2016-01-13 7:04 ` Bharata B Rao
0 siblings, 0 replies; 39+ messages in thread
From: Bharata B Rao @ 2016-01-13 7:04 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, aik, agraf, qemu-devel, pbonzini, qemu-ppc, tyreld,
nfont, imammedo, afaerber, ehabkost
On Wed, Jan 13, 2016 at 03:57:00PM +1100, David Gibson wrote:
> On Wed, Jan 13, 2016 at 09:40:54AM +0530, Bharata B Rao wrote:
> > On Tue, Jan 12, 2016 at 05:06:34PM +1100, David Gibson wrote:
> > > On Fri, Jan 08, 2016 at 12:25:19PM +0530, Bharata B Rao wrote:
> > > > Remove the CPU core device by removing the underlying CPU thread devices.
> > > > Support hot removal of CPU for sPAPR guests by sending the hot unplug
> > > > notification to the guest via EPOW interrupt. Release the vCPU object
> > > > after CPU hot unplug so that vCPU fd can be parked and reused.
> > > >
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > > hw/ppc/spapr.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > include/hw/ppc/spapr.h | 6 +++
> > > > 2 files changed, 119 insertions(+)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index c2af9ca..43cb726 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -93,6 +93,9 @@
> > > >
> > > > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
> > > >
> > > > +static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list
> > > > + = QLIST_HEAD_INITIALIZER(cpu_unplug_list);
> > > > +
> > > > static XICSState *try_create_xics(const char *type, int nr_servers,
> > > > int nr_irqs, Error **errp)
> > > > {
> > > > @@ -2432,11 +2435,121 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > > }
> > > > }
> > > >
> > > > +static void spapr_cpu_destroy(PowerPCCPU *cpu)
> > > > +{
> > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > +
> > > > + xics_cpu_destroy(spapr->icp, cpu);
> > > > + qemu_unregister_reset(spapr_cpu_reset, cpu);
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_cleanup(void)
> > > > +{
> > > > + sPAPRCPUUnplug *unplug, *next;
> > > > + Object *cpu;
> > > > +
> > > > + QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) {
> > > > + cpu = unplug->cpu;
> > > > + object_unparent(cpu);
> > > > + QLIST_REMOVE(unplug, node);
> > > > + g_free(unplug);
> > > > + }
> > > > +}
> > > > +
> > > > +static void spapr_add_cpu_to_unplug_list(Object *cpu)
> > > > +{
> > > > + sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
> > > > +
> > > > + unplug->cpu = cpu;
> > > > + QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node);
> > > > +}
> > > > +
> > > > +static int spapr_cpu_release(Object *obj, void *opaque)
> > > > +{
> > > > + DeviceState *dev = DEVICE(obj);
> > > > + CPUState *cs = CPU(dev);
> > > > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > > +
> > > > + spapr_cpu_destroy(cpu);
> > > > + cpu_remove_sync(cs);
> > > > +
> > > > + /*
> > > > + * We are still walking the core object's children list, and
> > > > + * hence can't cleanup this CPU thread object just yet. Put
> > > > + * it on a list for later removal.
> > > > + */
> > > > + spapr_add_cpu_to_unplug_list(obj);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void spapr_core_release(DeviceState *dev, void *opaque)
> > > > +{
> > > > + object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque);
> > > > + spapr_cpu_core_cleanup();
> > > > + object_unparent(OBJECT(dev));
> > >
> > > I'd prefer to see the unplug_list as a local to this function (passed
> > > to spapr_cpu_release via opaque) rather than using a new global.
> >
> > Will try that in the next iteration.
> >
> > >
> > > > +}
> > > > +
> > > > +static int spapr_core_detach(Object *obj, void *opaque)
> > > > +{
> > > > + sPAPRCoreState *core = opaque;
> > > > + DeviceState *dev = DEVICE(obj);
> > > > + CPUState *cs = CPU(dev);
> > > > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > > + int id = ppc_get_vcpu_dt_id(cpu);
> > > > + int smt = kvmppc_smt_threads();
> > > > + sPAPRDRConnector *drc =
> > > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> > > > + sPAPRDRConnectorClass *drck;
> > > > + Error *local_err = NULL;
> > > > +
> > > > + /*
> > > > + * SMT threads return from here, only main thread (thread 0) will
> > > > + * continue and signal hot unplug event to the guest.
> > > > + */
> > >
> > > This seems silly. spapr_core_unplug() iterates through all the
> > > children only to have all of them except thread 0 ignore the call..
> > >
> > > Can't you just grab the first thread and do this logic directly from
> > > spapr_core_unplug()?
> >
> > If I purely rely on the children list of the core object like I am doing
> > here, I don't see how to grab the first thread object from the list w/o
> > doing what I am doing here. However I can store the first thread object
> > in the core object during the core object's instance_init and use it here.
> > Will give it a try in the next iteration.
>
> It should be accessible by name as "thread[0]" no?
Yes, like this AFAICS:
ObjectProperty *prop = object_property_find(OBJECT(core), "thread[0]", &local_err);
Object *thread = prop->opaque;
Storing the object pointer of the main thread in the core looks simpler to
me.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 39+ messages in thread