Re: [Qemu-devel] [PATCH v6 01/11] machine: Don't allow CPU toplogies with partially filled cores

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]