Re: [Qemu-devel] [PATCH v8 02/11] numa: move numa global variable nb_numa_nodes into MachineState

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 29 Jul 2019 14:31:18 +0800
Tao Xu <tao3.xu@intel.com> wrote:

> Add struct NumaState in MachineState and move existing numa global
> nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> numa_support into MachineClass to decide which submachines support NUMA.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
> 
> Changes in v8:
>     - Add check if numa->numa_state is NULL in pxb_dev_realize_common
>     - Use nb_nodes in spapr_populate_memory() (Igor)
> ---
[...]
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 821f0d4a49..1c7c12c415 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -331,7 +331,7 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
>              return ret;
>          }
>  
> -        if (nb_numa_nodes > 1) {
> +        if (ms->numa_state->num_nodes > 1) {
>              ret = spapr_fixup_cpu_numa_dt(fdt, offset, cpu);
>              if (ret < 0) {
>                  return ret;
> @@ -351,9 +351,9 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
>  
>  static hwaddr spapr_node0_size(MachineState *machine)
>  {
> -    if (nb_numa_nodes) {
> +    if (machine->numa_state->num_nodes) {
>          int i;
> -        for (i = 0; i < nb_numa_nodes; ++i) {
> +        for (i = 0; i < machine->numa_state->num_nodes; ++i) {
>              if (numa_info[i].node_mem) {
>                  return MIN(pow2floor(numa_info[i].node_mem),
>                             machine->ram_size);
> @@ -398,13 +398,14 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
>      hwaddr mem_start, node_size;
> -    int i, nb_nodes = nb_numa_nodes;
> +    int i, nb_nodes = machine->numa_state->num_nodes;
>      NodeInfo *nodes = numa_info;
>      NodeInfo ramnode;
>  
>      /* No NUMA nodes, assume there is just one node with whole RAM */
> -    if (!nb_numa_nodes) {
> +    if (!nb_nodes) {
>          nb_nodes = 1;
> +        machine->numa_state->num_nodes = nb_nodes;
You've not addressed a v7 comment
On Tue, 23 Jul 2019 16:56:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> I don't like user fixing up generic machine data that came from CLI
> (or luck of such)
[...]
> I'd keep fixup local (i.e. using nb_nodes)

i.e. do not modify machine->numa_state->num_nodes and just use value local
like current code does.

>          ramnode.node_mem = machine->ram_size;
>          nodes = &ramnode;
>      }
[...]