[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 8/9] machine: Refactor smp_parse() in vl.c as
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v3 8/9] machine: Refactor smp_parse() in vl.c as MachineClass::smp_parse() |
Date: |
Wed, 19 Jun 2019 16:24:06 -0300 |
On Wed, Jun 12, 2019 at 04:41:03PM +0800, Like Xu wrote:
> To make smp_parse() more flexible and expansive, a smp_parse function
> pointer is added to MachineClass that machine types could override.
>
> The generic smp_parse() code in vl.c is moved to hw/core/machine.c, and
> become the default implementation of MachineClass::smp_parse. A PC-specific
> function called pc_smp_parse() has been added to hw/i386/pc.c, which in
> this patch changes nothing against the default one .
>
> Suggested-by: Eduardo Habkost <address@hidden>
> Signed-off-by: Like Xu <address@hidden>
> ---
> hw/core/machine.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
> hw/i386/pc.c | 76 +++++++++++++++++++++++++++++++++++++++++++
> include/hw/boards.h | 5 +++
> include/hw/i386/pc.h | 1 +
> vl.c | 75 ++----------------------------------------
> 5 files changed, 161 insertions(+), 73 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9eeba448ed..d58a684abf 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -11,6 +11,9 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/option.h"
> +#include "qapi/qmp/qerror.h"
> +#include "sysemu/replay.h"
> #include "qemu/units.h"
> #include "hw/boards.h"
> #include "qapi/error.h"
> @@ -722,6 +725,79 @@ void machine_set_cpu_numa_node(MachineState *machine,
> }
> }
>
> +static void smp_parse(MachineState *ms, QemuOpts *opts)
> +{
> + /* copy it from legacy smp_parse() in vl.c */
This comment stops making sense for people reading the code as
soon as we delete smp_parse() from vl.c. Was it kept by
accident?
> + if (opts) {
> + unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
> + unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> + unsigned cores = qemu_opt_get_number(opts, "cores", 0);
> + unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> +
> + /* compute missing values, prefer sockets over cores over threads */
> + if (cpus == 0 || sockets == 0) {
> + cores = cores > 0 ? cores : 1;
> + threads = threads > 0 ? threads : 1;
> + if (cpus == 0) {
> + sockets = sockets > 0 ? sockets : 1;
> + cpus = cores * threads * sockets;
> + } else {
> + ms->smp.max_cpus =
> + qemu_opt_get_number(opts, "maxcpus", cpus);
> + sockets = ms->smp.max_cpus / (cores * threads);
> + }
> + } else if (cores == 0) {
> + threads = threads > 0 ? threads : 1;
> + cores = cpus / (sockets * threads);
> + cores = cores > 0 ? cores : 1;
> + } else if (threads == 0) {
> + threads = cpus / (cores * sockets);
> + threads = threads > 0 ? threads : 1;
> + } else if (sockets * cores * threads < cpus) {
> + error_report("cpu topology: "
> + "sockets (%u) * cores (%u) * threads (%u) < "
> + "smp_cpus (%u)",
> + sockets, cores, threads, cpus);
> + exit(1);
> + }
> +
> + ms->smp.max_cpus =
> + qemu_opt_get_number(opts, "maxcpus", cpus);
> +
> + if (ms->smp.max_cpus < cpus) {
> + error_report("maxcpus must be equal to or greater than smp");
> + exit(1);
> + }
> +
> + if (sockets * cores * threads > ms->smp.max_cpus) {
> + error_report("cpu topology: "
> + "sockets (%u) * cores (%u) * threads (%u) > "
> + "maxcpus (%u)",
> + sockets, cores, threads,
> + ms->smp.max_cpus);
> + exit(1);
> + }
> +
> + if (sockets * cores * threads != ms->smp.max_cpus) {
> + warn_report("Invalid CPU topology deprecated: "
> + "sockets (%u) * cores (%u) * threads (%u) "
> + "!= maxcpus (%u)",
> + sockets, cores, threads,
> + ms->smp.max_cpus);
> + }
> +
> + ms->smp.cpus = cpus;
> + ms->smp.cores = cores;
> + ms->smp.threads = threads;
> + }
> +
> + if (ms->smp.cpus > 1) {
> + Error *blocker = NULL;
> + error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
> + replay_add_blocker(blocker);
> + }
> +}
> +
> static void machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -729,6 +805,7 @@ static void machine_class_init(ObjectClass *oc, void
> *data)
> /* Default 128 MB as guest ram size */
> mc->default_ram_size = 128 * MiB;
> mc->rom_file_has_mr = true;
> + mc->smp_parse = smp_parse;
>
> /* numa node memory size aligned on 8MB by default.
> * On Linux, each node's border has to be 8MB aligned
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b4dbd1064d..63b44bd2bd 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -78,6 +78,8 @@
> #include "hw/i386/intel_iommu.h"
> #include "hw/net/ne2000-isa.h"
> #include "standard-headers/asm-x86/bootparam.h"
> +#include "sysemu/replay.h"
> +#include "qapi/qmp/qerror.h"
>
> /* debug PC/ISA interrupts */
> //#define DEBUG_IRQ
> @@ -1539,6 +1541,79 @@ static void pc_new_cpu(PCMachineState *pcms, int64_t
> apic_id, Error **errp)
> error_propagate(errp, local_err);
> }
>
> +void pc_smp_parse(MachineState *ms, QemuOpts *opts)
> +{
> + /* copy it from legacy smp_parse() in vl.c */
I suggest replacing this comment with one saying this function is
very similar to smp_parse() in hw/core/machine.c but includes CPU
die support.
I can remove the comment while committing, and the comment may be
submitted as a follow up patch.
Reviewed-by: Eduardo Habkost <address@hidden>
> + if (opts) {
> + unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
> + unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> + unsigned cores = qemu_opt_get_number(opts, "cores", 0);
> + unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> +
> + /* compute missing values, prefer sockets over cores over threads */
> + if (cpus == 0 || sockets == 0) {
> + cores = cores > 0 ? cores : 1;
> + threads = threads > 0 ? threads : 1;
> + if (cpus == 0) {
> + sockets = sockets > 0 ? sockets : 1;
> + cpus = cores * threads * sockets;
> + } else {
> + ms->smp.max_cpus =
> + qemu_opt_get_number(opts, "maxcpus", cpus);
> + sockets = ms->smp.max_cpus / (cores * threads);
> + }
> + } else if (cores == 0) {
> + threads = threads > 0 ? threads : 1;
> + cores = cpus / (sockets * threads);
> + cores = cores > 0 ? cores : 1;
> + } else if (threads == 0) {
> + threads = cpus / (cores * sockets);
> + threads = threads > 0 ? threads : 1;
> + } else if (sockets * cores * threads < cpus) {
> + error_report("cpu topology: "
> + "sockets (%u) * cores (%u) * threads (%u) < "
> + "smp_cpus (%u)",
> + sockets, cores, threads, cpus);
> + exit(1);
> + }
> +
> + ms->smp.max_cpus =
> + qemu_opt_get_number(opts, "maxcpus", cpus);
> +
> + if (ms->smp.max_cpus < cpus) {
> + error_report("maxcpus must be equal to or greater than smp");
> + exit(1);
> + }
> +
> + if (sockets * cores * threads > ms->smp.max_cpus) {
> + error_report("cpu topology: "
> + "sockets (%u) * cores (%u) * threads (%u) > "
> + "maxcpus (%u)",
> + sockets, cores, threads,
> + ms->smp.max_cpus);
> + exit(1);
> + }
> +
> + if (sockets * cores * threads != ms->smp.max_cpus) {
> + warn_report("Invalid CPU topology deprecated: "
> + "sockets (%u) * cores (%u) * threads (%u) "
> + "!= maxcpus (%u)",
> + sockets, cores, threads,
> + ms->smp.max_cpus);
> + }
> +
> + ms->smp.cpus = cpus;
> + ms->smp.cores = cores;
> + ms->smp.threads = threads;
> + }
> +
> + if (ms->smp.cpus > 1) {
> + Error *blocker = NULL;
> + error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
> + replay_add_blocker(blocker);
> + }
> +}
> +
> void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp)
> {
> int64_t apic_id = x86_cpu_apic_id_from_index(ms, id);
> @@ -2779,6 +2854,7 @@ static void pc_machine_class_init(ObjectClass *oc, void
> *data)
> mc->has_hotpluggable_cpus = true;
> mc->default_boot_order = "cad";
> mc->hot_add_cpu = pc_hot_add_cpu;
> + mc->smp_parse = pc_smp_parse;
> mc->block_default_type = IF_IDE;
> mc->max_cpus = 255;
> mc->reset = pc_machine_reset;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1e000229e1..c025f33407 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -158,6 +158,10 @@ typedef struct {
> * @kvm_type:
> * Return the type of KVM corresponding to the kvm-type string option or
> * computed based on other criteria such as the host kernel capabilities.
> + * @smp_parse:
> + * The function pointer to hook different machine specific functions for
> + * parsing "smp-opts" from QemuOpts to MachineState::CpuTopology and more
> + * machine specific topology fields, such as smp_dies for PCMachine.
> */
> struct MachineClass {
> /*< private >*/
> @@ -174,6 +178,7 @@ struct MachineClass {
> void (*reset)(MachineState *state);
> void (*hot_add_cpu)(MachineState *state, const int64_t id, Error **errp);
> int (*kvm_type)(MachineState *machine, const char *arg);
> + void (*smp_parse)(MachineState *ms, QemuOpts *opts);
>
> BlockInterfaceType block_default_type;
> int units_per_default_bus;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index fae9217e34..7ca24746cc 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -189,6 +189,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level);
>
> void pc_cpus_init(PCMachineState *pcms);
> void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp);
> +void pc_smp_parse(MachineState *ms, QemuOpts *opts);
>
> void pc_guest_info_init(PCMachineState *pcms);
>
> diff --git a/vl.c b/vl.c
> index 0760b2724e..53ea9b6d6f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1245,78 +1245,6 @@ static QemuOptsList qemu_smp_opts = {
> },
> };
>
> -static void smp_parse(QemuOpts *opts)
> -{
> - if (opts) {
> - unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
> - unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> - unsigned cores = qemu_opt_get_number(opts, "cores", 0);
> - unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> -
> - /* compute missing values, prefer sockets over cores over threads */
> - if (cpus == 0 || sockets == 0) {
> - cores = cores > 0 ? cores : 1;
> - threads = threads > 0 ? threads : 1;
> - if (cpus == 0) {
> - sockets = sockets > 0 ? sockets : 1;
> - cpus = cores * threads * sockets;
> - } else {
> - current_machine->smp.max_cpus =
> - qemu_opt_get_number(opts, "maxcpus", cpus);
> - sockets = current_machine->smp.max_cpus / (cores * threads);
> - }
> - } else if (cores == 0) {
> - threads = threads > 0 ? threads : 1;
> - cores = cpus / (sockets * threads);
> - cores = cores > 0 ? cores : 1;
> - } else if (threads == 0) {
> - threads = cpus / (cores * sockets);
> - threads = threads > 0 ? threads : 1;
> - } else if (sockets * cores * threads < cpus) {
> - error_report("cpu topology: "
> - "sockets (%u) * cores (%u) * threads (%u) < "
> - "smp_cpus (%u)",
> - sockets, cores, threads, cpus);
> - exit(1);
> - }
> -
> - current_machine->smp.max_cpus =
> - qemu_opt_get_number(opts, "maxcpus", cpus);
> -
> - if (current_machine->smp.max_cpus < cpus) {
> - error_report("maxcpus must be equal to or greater than smp");
> - exit(1);
> - }
> -
> - if (sockets * cores * threads > current_machine->smp.max_cpus) {
> - error_report("cpu topology: "
> - "sockets (%u) * cores (%u) * threads (%u) > "
> - "maxcpus (%u)",
> - sockets, cores, threads,
> - current_machine->smp.max_cpus);
> - exit(1);
> - }
> -
> - if (sockets * cores * threads != current_machine->smp.max_cpus) {
> - warn_report("Invalid CPU topology deprecated: "
> - "sockets (%u) * cores (%u) * threads (%u) "
> - "!= maxcpus (%u)",
> - sockets, cores, threads,
> - current_machine->smp.max_cpus);
> - }
> -
> - current_machine->smp.cpus = cpus;
> - current_machine->smp.cores = cores;
> - current_machine->smp.threads = threads;
> - }
> -
> - if (current_machine->smp.cpus > 1) {
> - Error *blocker = NULL;
> - error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
> - replay_add_blocker(blocker);
> - }
> -}
> -
> static void realtime_init(void)
> {
> if (enable_mlock) {
> @@ -4043,7 +3971,8 @@ int main(int argc, char **argv, char **envp)
> current_machine->smp.cores = 1;
> current_machine->smp.threads = 1;
>
> - smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
> + machine_class->smp_parse(current_machine,
> + qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>
> /* sanity-check smp_cpus and max_cpus against machine_class */
> if (current_machine->smp.cpus < machine_class->min_cpus) {
> --
> 2.21.0
>
--
Eduardo
- Re: [Qemu-devel] [PATCH v3 2/9] hw/i386: Adjust nr_dies with configured smp_dies for PCMachine, (continued)
- [Qemu-devel] [PATCH v3 5/9] tests/x86-cpuid: Update testcases in test_topo_bits() with multiple dies, Like Xu, 2019/06/12
- [Qemu-devel] [PATCH v3 3/9] i386/cpu: Consolidate die-id validity in smp context, Like Xu, 2019/06/12
- [Qemu-devel] [PATCH v3 4/9] i386: Update new x86_apicid parsing rules with die_offset support, Like Xu, 2019/06/12
- [Qemu-devel] [PATCH v3 6/9] i386/cpu: Add CPUID.1F generation support for multi-dies PCMachine, Like Xu, 2019/06/12
- [Qemu-devel] [PATCH v3 8/9] machine: Refactor smp_parse() in vl.c as MachineClass::smp_parse(), Like Xu, 2019/06/12
- Re: [Qemu-devel] [PATCH v3 8/9] machine: Refactor smp_parse() in vl.c as MachineClass::smp_parse(),
Eduardo Habkost <=
- [Qemu-devel] [PATCH v3 9/9] vl.c: Add -smp, dies=* command line support and update doc, Like Xu, 2019/06/12
- [Qemu-devel] [PATCH v3 1/9] i386: Add die-level cpu topology to x86CPU on PCMachine, Like Xu, 2019/06/12
- Re: [Qemu-devel] [PATCH v3 0/9] Introduce cpu die topology and enable CPUID.1F for i386, Like Xu, 2019/06/18