Re: [Qemu-devel] [PATCH v7 05/11] numa: Extend CLI to provide initiator information for numa nodes

["Liu, Jingqi" <jingqi.liu@intel.com>]

> -----Original Message-----
> From: Xu, Tao3
> Sent: Tuesday, July 16, 2019 10:51 PM
> To: imammedo@redhat.com; eblake@redhat.com; ehabkost@redhat.com
> Cc: Xu, Tao3 <tao3.xu@intel.com>; Liu, Jingqi <jingqi.liu@intel.com>; Williams, Dan J <dan.j.williams@intel.com>;
> jonathan.cameron@huawei.com; Du, Fan <fan.du@intel.com>; qemu-devel@nongnu.org
> Subject: [PATCH v7 05/11] numa: Extend CLI to provide initiator information for numa nodes
> 
> In ACPI 6.3 chapter 5.2.27 Heterogeneous Memory Attribute Table (HMAT), The initiator represents processor which access to memory. And
> in 5.2.27.3 Memory Proximity Domain Attributes Structure, the attached initiator is defined as where the memory controller responsible for
> a memory proximity domain. With attached initiator information, the topology of heterogeneous memory can be described.
> 
> Extend CLI of "-numa node" option to indicate the initiator numa node-id.
> In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report the platform's HMAT tables.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
Looks good for me.

Reviewed-by: Jingqi Liu <Jingqi.liu@intel.com>

Thanks
Jingqi Liu
> ---
> 
> No changes in v7.
> 
> Changes in v6:
>     - Add the version designator (since 4.2) after @initiator (Eric)
> ---
>  hw/core/machine.c     | 24 ++++++++++++++++++++++++
>  hw/core/numa.c        | 13 +++++++++++++
>  include/sysemu/numa.h |  3 +++
>  qapi/machine.json     |  6 +++++-
>  qemu-options.hx       | 27 +++++++++++++++++++++++----
>  5 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c index 4228bcd2a2..063cb7923c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -653,6 +653,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>                                 const CpuInstanceProperties *props, Error **errp)  {
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    NodeInfo *numa_info = machine->numa_state->nodes;
>      bool match = false;
>      int i;
> 
> @@ -722,6 +723,16 @@ void machine_set_cpu_numa_node(MachineState *machine,
>          match = true;
>          slot->props.node_id = props->node_id;
>          slot->props.has_node_id = props->has_node_id;
> +
> +        if (numa_info[props->node_id].initiator_valid &&
> +            (props->node_id != numa_info[props->node_id].initiator)) {
> +            error_setg(errp, "The initiator of CPU NUMA node %" PRId64
> +                       " should be itself.", props->node_id);
> +            return;
> +        }
> +        numa_info[props->node_id].initiator_valid = true;
> +        numa_info[props->node_id].has_cpu = true;
> +        numa_info[props->node_id].initiator = props->node_id;
>      }
> 
>      if (!match) {
> @@ -1063,6 +1074,7 @@ static void machine_numa_finish_cpu_init(MachineState *machine)
>      GString *s = g_string_new(NULL);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
> +    NodeInfo *numa_info = machine->numa_state->nodes;
> 
>      assert(machine->numa_state->num_nodes);
>      for (i = 0; i < possible_cpus->len; i++) { @@ -1096,6 +1108,18 @@ static void machine_numa_finish_cpu_init(MachineState *machine)
>              machine_set_cpu_numa_node(machine, &props, &error_fatal);
>          }
>      }
> +
> +    for (i = 0; i < machine->numa_state->num_nodes; i++) {
> +        if (numa_info[i].initiator_valid &&
> +            !numa_info[numa_info[i].initiator].has_cpu) {
> +            error_report("The initiator-id %"PRIu16 " of NUMA node %d"
> +                         " does not exist.", numa_info[i].initiator, i);
> +            error_printf("\n");
> +
> +            exit(1);
> +        }
> +    }
> +
>      if (s->len && !qtest_enabled()) {
>          warn_report("CPU(s) not present in any NUMA nodes: %s",
>                      s->str);
> diff --git a/hw/core/numa.c b/hw/core/numa.c index 8fcbba05d6..cfb6339810 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -128,6 +128,19 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>          numa_info[nodenr].node_mem = object_property_get_uint(o, "size", NULL);
>          numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
>      }
> +
> +    if (node->has_initiator) {
> +        if (numa_info[nodenr].initiator_valid &&
> +            (node->initiator != numa_info[nodenr].initiator)) {
> +            error_setg(errp, "The initiator of NUMA node %" PRIu16 " has been "
> +                       "set to node %" PRIu16, nodenr,
> +                       numa_info[nodenr].initiator);
> +            return;
> +        }
> +
> +        numa_info[nodenr].initiator_valid = true;
> +        numa_info[nodenr].initiator = node->initiator;
> +    }
>      numa_info[nodenr].present = true;
>      max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
>      ms->numa_state->num_nodes++;
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h index 76da3016db..46ad06e000 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -10,6 +10,9 @@ struct NodeInfo {
>      uint64_t node_mem;
>      struct HostMemoryBackend *node_memdev;
>      bool present;
> +    bool has_cpu;
> +    bool initiator_valid;
> +    uint16_t initiator;
>      uint8_t distance[MAX_NODES];
>  };
> 
> diff --git a/qapi/machine.json b/qapi/machine.json index 6db8a7e2ec..05e367d26a 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -414,6 +414,9 @@
>  # @memdev: memory backend object.  If specified for one node,
>  #          it must be specified for all nodes.
>  #
> +# @initiator: the initiator numa nodeid that is closest (as in directly
> +#             attached) to this numa node (since 4.2)
> +#
>  # Since: 2.1
>  ##
>  { 'struct': 'NumaNodeOptions',
> @@ -421,7 +424,8 @@
>     '*nodeid': 'uint16',
>     '*cpus':   ['uint16'],
>     '*mem':    'size',
> -   '*memdev': 'str' }}
> +   '*memdev': 'str',
> +   '*initiator': 'uint16' }}
> 
>  ##
>  # @NumaDistOptions:
> diff --git a/qemu-options.hx b/qemu-options.hx index 9621e934c0..c480781992 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -161,14 +161,14 @@ If any on the three values is given, the total number of CPUs @var{n} can be omi  ETEXI
> 
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> -    "-numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node]\n"
> -    "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node]\n"
> +    "-numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
> +    "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
>      "-numa dist,src=source,dst=destination,val=distance\n"
>      "-numa cpu,node-id=node[,socket-id=x][,core-id=y][,thread-id=z]\n",
>      QEMU_ARCH_ALL)
>  STEXI
> -@item -numa node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}]
> -@itemx -numa node[,memdev=@var{id}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}]
> +@item -numa
> +node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@va
> +r{node}][,initiator=@var{initiator}]
> +@itemx -numa
> +node[,memdev=@var{id}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@v
> +ar{node}][,initiator=@var{initiator}]
>  @itemx -numa dist,src=@var{source},dst=@var{destination},val=@var{distance}
>  @itemx -numa cpu,node-id=@var{node}[,socket-id=@var{x}][,core-id=@var{y}][,thread-id=@var{z}]
>  @findex -numa
> @@ -215,6 +215,25 @@ split equally between them.
>  @samp{mem} and @samp{memdev} are mutually exclusive. Furthermore,  if one node uses @samp{memdev}, all of them have to use it.
> 
> +@samp{initiator} indicate the initiator NUMA @var{initiator} that is
> +closest (as in directly attached) to this NUMA @var{node}.
> +
> +For example, the following option assigns 2 NUMA nodes, node 0 has CPU.
> +node 1 has only memory, and its' initiator is node 0. Note that because
> +node 0 has CPU, by default the initiator of node 0 is itself and must
> +be itself.
> +@example
> +-M pc \
> +-m 2G,slots=2,maxmem=4G \
> +-object memory-backend-ram,size=1G,id=m0 \ -object
> +memory-backend-ram,size=1G,id=m1 \ -numa node,nodeid=0,memdev=m0 \
> +-numa node,nodeid=1,memdev=m1,initiator=0 \ -smp 2,sockets=2,maxcpus=2
> +\ -numa cpu,node-id=0,socket-id=0 \ -numa cpu,node-id=0,socket-id=1 \
> +@end example
> +
>  @var{source} and @var{destination} are NUMA node IDs.
>  @var{distance} is the NUMA distance from @var{source} to @var{destination}.
>  The distance from a node to itself is always 10. If any pair of nodes is
> --
> 2.20.1