Re: [Qemu-devel] [PATCH v3 8/9] machine: Refactor smp_parse() in vl.c as MachineClass::smp_parse()

[Eduardo Habkost]

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