qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] numa: numa nodeid need not be sequential


From: David Gibson
Subject: Re: [Qemu-devel] [PATCH v2] numa: numa nodeid need not be sequential
Date: Wed, 7 Aug 2019 20:03:34 +1000
User-agent: Mutt/1.12.1 (2019-06-15)

On Tue, Aug 06, 2019 at 09:29:45PM +1000, Daniel Black wrote:
> Replace all node_id assumptions with lookups from 
> machinestate->numa_state->nodes[]
> and remove aspects that assume a sequential numbering of nodes. This enables
> non-sequential NUMA node number topoligies to be created.
> 
> Default assignments of CPU->nodeid (get_default_cpu_node_id) now return
> a nodeid from the machinestate->numa_state->nodes[].
> 
> x86 will use the node is as the Proximity Domain (which the
> Linux kernel will map down to sequential node numbers). Both HMAT and
> SLIT ACPI data are entered based on this nodeid. In Linux kernel
> output look at the SRAT/HMAT: and PXM: references in the kernel early boot.
> 
> Small enhancements where made to error messages to be more explicit
> about errors in node specification.
> 
> CC: Tao Xu <address@hidden>
> CC: Liu Jingqi <address@hidden>
> Signed-off-by: Daniel Black <address@hidden>

I have no real opinion on whether this is a good idea overall.  But,
if we go for it then the ppc parts are

Acked-by: David Gibson <address@hidden>

> 
> ---
> Based-on: address@hidden
> ([PATCH RESEND v8 00/11] Build ACPI Heterogeneous Memory Attribute Table
> (HMAT))
> 
> Test script:
> 
> #!/bin/bash
> set -x -v
> 
> QEMUHOME=${HOME}/repos/qemu/
> # optional but make it easy to install/run numactl --hardware
> #ALPINE_NET=""
> ALPINE_NET="ip=dhcp 
> alpine_repo=http://dl-cdn.alpinelinux.org/alpine/edge/main/";
> 
> ALPINE_HOME=${HOME}/repos/alpine/alpine-netboot-3.10.1-
> 
> # x86 / armv7 - no CONFIG_NUMA=y support in kernel
> # Kernel configs: https://git.alpinelinux.org/aports/tree/main/linux-vanilla/
> # s390x - no numa support in QEMU
> for ARCH in x86_64 aarch64 ppc64le
> do
>  if [ ! -d ${ALPINE_HOME}${ARCH} ]
>  then
>    mkdir ${ALPINE_HOME}${ARCH}
>    wget 
> http://dl-cdn.alpinelinux.org/alpine/v3.10/releases/${ARCH}/alpine-netboot-3.10.1-${ARCH}.tar.gz
>  -O - | tar -zxf - -C ${ALPINE_HOME}${ARCH}
>  fi
> done
> 
> if [ ! -x ${ALPINE_HOME}i386 ]
> then
>   ln -s ${ALPINE_HOME}x86 ${ALPINE_HOME}i386
> fi
> 
> if [ ! -x ${ALPINE_HOME}arm ]
> then
>   ln -s ${ALPINE_HOME}armv7 ${ALPINE_HOME}arm
> fi
> 
> if [ ! -x ${ALPINE_HOME}ppc64 ]
> then
>   ln -s ${ALPINE_HOME}ppc64le ${ALPINE_HOME}ppc64
> fi
> 
> # Note "virtual" kernels don't have numa enabled
> run()
> {
>   NUMA=$1
>   ARCH=$2
>   ARGS=$3
>   CONSOLE=$4
>   #echo \
>   ${QEMUHOME}/${ARCH}-softmmu/qemu-system-${ARCH} \
>     ${ARGS} \
>     -kernel ${ALPINE_HOME}${ARCH}/boot/vmlinuz-vanilla  \
>     -initrd ${ALPINE_HOME}${ARCH}/boot/initramfs-vanilla \
>     -append "${CONSOLE} ${ALPINE_NET}" \
>     -m 2G \
>     ${NUMA}
>   echo
> }
> 
> # This ends up as odd:
> # ends up with both CPUs are on same node
> # as 0 and 8 % 2 (nodes) are the same
> # in short - don't run legacy with gaps with
> # odd numa node numbers (like 0 and 8).
> run_legacy()
> {
>   run "-smp 2,cores=3,sockets=2,maxcpus=6 \
>        -numa node,mem=1G \
>        -numa node,mem=1G,nodeid=8 \
>        -numa dist,src=0,dst=8,val=21" "$@"
> }
> 
> run_memdev_implicit_core()
> {
>   run "-smp cpus=6,maxcpus=8,cores=4,sockets=2 \
>        -object memory-backend-ram,id=ram0,size=1G \
>        -object memory-backend-ram,id=ram1,size=1G \
>        -numa node,memdev=ram0,nodeid=0 \
>        -numa node,memdev=ram1,nodeid=8 \
>        -numa dist,src=0,dst=8,val=21" "$@"
> }
> 
> run_memdev_explicit_core()
> {
>   run "-smp cpus=6,maxcpus=8,cores=4,sockets=2 \
>        -object memory-backend-ram,id=ram0,size=1G \
>        -object memory-backend-ram,id=ram1,size=1G \
>        -numa node,memdev=ram0,cpus=0-3,nodeid=0 \
>        -numa node,memdev=ram1,cpus=4-7,nodeid=8 \
>        -numa dist,src=0,dst=8,val=21" "$@"
> }
> 
> # hmat isn't added until kernel-5.2-rc1 and requires
> # CONFIG_ACPI_HMAT
> run_hmat_lb()
> {
>   run "-smp 2,sockets=2 \
>        -m 128M,slots=2,maxmem=1G \
>        -kernel ${HOME}/repos/linux/vmlinux \
>        -object memory-backend-ram,size=64M,id=m0 \
>        -object memory-backend-ram,size=64M,id=m1 \
>        -numa node,nodeid=3,memdev=m0 \
>        -numa node,nodeid=4,memdev=m1,initiator=3 \
>        -numa cpu,node-id=3,socket-id=0 \
>        -numa cpu,node-id=3,socket-id=1 \
>        -numa 
> hmat-lb,initiator=3,target=3,hierarchy=memory,data-type=access-latency,base-lat=1000,latency=5
>  \
>        -numa 
> hmat-lb,initiator=3,target=3,hierarchy=memory,data-type=access-bandwidth,base-bw=20,bandwidth=5
>  \
>        -numa 
> hmat-lb,initiator=3,target=4,hierarchy=memory,data-type=access-latency,base-lat=1,latency=15
>  \
>        -numa 
> hmat-lb,initiator=3,target=4,hierarchy=memory,data-type=access-bandwidth,base-bw=20,bandwidth=10
>  \
>        -numa 
> hmat-cache,node-id=3,size=0x20000,total=1,level=1,assoc=direct,policy=write-back,line=8
>  \
>        -numa 
> hmat-cache,node-id=4,size=0x20000,total=1,level=1,assoc=direct,policy=write-back,line=8"
>  "$@"
> }
> 
> 
> for arch in x86_64 ppc64 aarch64 s390x; do killall qemu-system-$arch; done
> killall vncviewer
> 
> # i386 Alpine kernels don't have NUMA
> #run_memdev_implicit_core i386 "-machine pc -nographic" console=ttyS0
> # armv7 kernel's don't have NUMA
> #run_legacy arm "-machine virt -cpu cortex-a15 -nographic" console=ttyAMA0
> 
> # GOOD
> run_legacy x86_64 "-machine pc -nographic" console=ttyS0
> run_memdev_implicit_core x86_64 "-machine pc -nographic" console=ttyS0
> run_memdev_explicit_core x86_64 "-machine pc -nographic" console=ttyS0
> 
> run_hmat_lb x86_64 "-machine pc -nographic" console=ttyS0
> 
> # GOOD
> run_legacy aarch64 "-machine virt -cpu cortex-a57 -nographic" console=ttyAMA0
> run_memdev_implicit_core  aarch64 "-machine virt -cpu cortex-a57 -nographic" 
> console=ttyAMA0
> run_memdev_explicit_core  aarch64 "-machine virt -cpu cortex-a57 -nographic" 
> console=ttyAMA0
> 
> # PPC not doing numa distance (not a regression)
> (sleep 1; vncviewer :0) &
> 
> # GOOD
>  run_legacy ppc64 "-machine pseries -cpu POWER9 -display vnc=:0" "numa=debug"
>  run_memdev_implicit_core ppc64 "-machine pseries -cpu POWER9 -display 
> vnc=:0" "numa=debug"
>  run_memdev_explicit_core ppc64 "-machine pseries -cpu POWER9 -display 
> vnc=:0" "numa=debug"
> 
> # ON P8 ppc64le host:
> # run_memdev_implicit_core ppc64 "-machine pseries -cpu host -accel kvm 
> -display vnc=:0" "numa=debug"
> 
> # Couldn't boot Alpine ARM kernel on this machine type:
> # arm sbsa ref - appears to be a BMC so not really a numa target?
> # seems ok looking at the results of sbsa_ref_get_default_cpu_node_id however 
> it display no
> # output when booting
> 
> # run_legacy aarch64 "-machine sbsa-ref -nographic" console=ttyAMA0
> 
> # Then run:
> # sh -c 'apk add numactl-tools && numactl --hardware'
> #
> # alternately examine results in:
> # ls -la /sys/devices/system/node/node*/cpu*
> # more /sys/devices/system/node/node*/distance
> #
> # x86 node numbers are renumbered by kernel. To view
> # acpi mappings:
> # dmesg | egrep -A 2 '(HMAT|SRAT|PXM):'
> ---
>  hw/acpi/aml-build.c      |  31 ++++++---
>  hw/acpi/hmat.c           |  14 +++--
>  hw/arm/boot.c            |   3 +-
>  hw/arm/sbsa-ref.c        |   6 +-
>  hw/arm/virt-acpi-build.c |   3 +-
>  hw/arm/virt.c            |   6 +-
>  hw/core/machine.c        |  40 +++++++++---
>  hw/core/numa.c           | 132 +++++++++++++++++++--------------------
>  hw/i386/acpi-build.c     |  12 ++--
>  hw/i386/pc.c             |   2 +-
>  hw/ppc/spapr.c           |  12 ++--
>  include/sysemu/numa.h    |   2 +
>  12 files changed, 154 insertions(+), 109 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 26ccc1a3e2..512c76e3dd 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1728,19 +1728,34 @@ void build_srat_memory(AcpiSratMemoryAffinity 
> *numamem, uint64_t base,
>   */
>  void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
>  {
> -    int slit_start, i, j;
> +    int slit_start, i, j, src, dst, largest;
>      slit_start = table_data->len;
>      int nb_numa_nodes = ms->numa_state->num_nodes;
>  
>      acpi_data_push(table_data, sizeof(AcpiTableHeader));
>  
> -    build_append_int_noprefix(table_data, nb_numa_nodes, 8);
> -    for (i = 0; i < nb_numa_nodes; i++) {
> -        for (j = 0; j < nb_numa_nodes; j++) {
> -            assert(ms->numa_state->nodes[i].distance[j]);
> -            build_append_int_noprefix(table_data,
> -                                      ms->numa_state->nodes[i].distance[j],
> -                                      1);
> +    for (largest = 0, i = 0; i < nb_numa_nodes; i++)
> +        if (largest < ms->numa_state->nodes[i].nodeid) {
> +            largest = ms->numa_state->nodes[i].nodeid;
> +        }
> +
> +    /* number of entries is largest + 1 as nodes start at 0 */
> +    build_append_int_noprefix(table_data, largest + 1, 8);
> +
> +    for (i = 0; i <= largest; i++) {
> +        src = find_numa(i, ms->numa_state);
> +        for (j = 0; j <= largest; j++) {
> +            dst = find_numa(j, ms->numa_state);
> +
> +            if (dst == MAX_NODES || src == MAX_NODES) {
> +                /* 255 is unreachable. Linux expects 10 in self-maps entries 
> */
> +                build_append_int_noprefix(table_data,
> +                                          i == j ? NUMA_DISTANCE_MIN : 255, 
> 1);
> +            } else {
> +                assert(ms->numa_state->nodes[src].distance[dst]);
> +                build_append_int_noprefix(table_data,
> +                    ms->numa_state->nodes[src].distance[dst], 1);
> +            }
>          }
>      }
>  
> diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
> index 01a6552d51..0042be48d2 100644
> --- a/hw/acpi/hmat.c
> +++ b/hw/acpi/hmat.c
> @@ -73,7 +73,8 @@ static void build_hmat_mpda(GArray *table_data, uint16_t 
> flags, int initiator,
>   */
>  static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
>                            uint32_t num_initiator, uint32_t num_target,
> -                          uint32_t *initiator_pxm, int type)
> +                          uint32_t *initiator_pxm, int type,
> +                          NumaState *numa_state)
>  {
>      uint32_t s = num_initiator;
>      uint32_t t = num_target;
> @@ -114,12 +115,12 @@ static void build_hmat_lb(GArray *table_data, 
> HMAT_LB_Info *hmat_lb,
>  
>      /* Target Proximity Domain List */
>      for (i = 0; i < t; i++) {
> -        build_append_int_noprefix(table_data, i, 4);
> +        build_append_int_noprefix(table_data, numa_state->nodes[i].nodeid, 
> 4);
>      }
>  
>      /* Latency or Bandwidth Entries */
>      for (i = 0; i < s; i++) {
> -        m = initiator_pxm[i];
> +        m = find_numa(initiator_pxm[i], numa_state);
>          for (n = 0; n < t; n++) {
>              uint16_t entry;
>  
> @@ -199,12 +200,13 @@ static void hmat_build_table_structs(GArray 
> *table_data, NumaState *nstat)
>              flags |= HMAT_PROX_INIT_VALID;
>          }
>  
> -        build_hmat_mpda(table_data, flags, nstat->nodes[i].initiator, i);
> +        build_hmat_mpda(table_data, flags, nstat->nodes[i].initiator,
> +                        nstat->nodes[i].nodeid);
>      }
>  
>      for (i = 0; i < nstat->num_nodes; i++) {
>          if (nstat->nodes[i].has_cpu) {
> -            initiator_pxm[num_initiator++] = i;
> +            initiator_pxm[num_initiator++] = nstat->nodes[i].nodeid;
>          }
>      }
>  
> @@ -220,7 +222,7 @@ static void hmat_build_table_structs(GArray *table_data, 
> NumaState *nstat)
>  
>              if (numa_hmat_lb) {
>                  build_hmat_lb(table_data, numa_hmat_lb, num_initiator,
> -                              nstat->num_nodes, initiator_pxm, type);
> +                              nstat->num_nodes, initiator_pxm, type, nstat);
>              }
>          }
>      }
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 6472aa441e..1d92001930 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -603,7 +603,8 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info 
> *binfo,
>          for (i = 0; i < ms->numa_state->num_nodes; i++) {
>              mem_len = ms->numa_state->nodes[i].node_mem;
>              rc = fdt_add_memory_node(fdt, acells, mem_base,
> -                                     scells, mem_len, i);
> +                                     scells, mem_len,
> +                                     ms->numa_state->nodes[i].nodeid);
>              if (rc < 0) {
>                  fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
>                          mem_base);
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 3a243e6a53..f2c3a6fefa 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -166,8 +166,8 @@ static void create_fdt(SBSAMachineState *sms)
>          for (i = 0; i < nb_numa_nodes; i++) {
>              for (j = 0; j < nb_numa_nodes; j++) {
>                  idx = (i * nb_numa_nodes + j) * 3;
> -                matrix[idx + 0] = cpu_to_be32(i);
> -                matrix[idx + 1] = cpu_to_be32(j);
> +                matrix[idx + 0] = 
> cpu_to_be32(ms->numa_state->nodes[i].nodeid);
> +                matrix[idx + 1] = 
> cpu_to_be32(ms->numa_state->nodes[j].nodeid);
>                  matrix[idx + 2] =
>                      cpu_to_be32(ms->numa_state->nodes[i].distance[j]);
>              }
> @@ -762,7 +762,7 @@ sbsa_ref_cpu_index_to_props(MachineState *ms, unsigned 
> cpu_index)
>  static int64_t
>  sbsa_ref_get_default_cpu_node_id(const MachineState *ms, int idx)
>  {
> -    return idx % ms->numa_state->num_nodes;
> +    return ms->numa_state->nodes[idx % ms->numa_state->num_nodes].nodeid;
>  }
>  
>  static void sbsa_ref_instance_init(Object *obj)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 89899ec4c1..0384339867 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -537,7 +537,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          if (ms->numa_state->nodes[i].node_mem > 0) {
>              numamem = acpi_data_push(table_data, sizeof(*numamem));
>              build_srat_memory(numamem, mem_base,
> -                              ms->numa_state->nodes[i].node_mem, i,
> +                              ms->numa_state->nodes[i].node_mem,
> +                              ms->numa_state->nodes[i].nodeid,
>                                MEM_AFFINITY_ENABLED);
>              mem_base += ms->numa_state->nodes[i].node_mem;
>          }
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 46f39e20bc..1a2db6447f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -240,8 +240,8 @@ static void create_fdt(VirtMachineState *vms)
>          for (i = 0; i < nb_numa_nodes; i++) {
>              for (j = 0; j < nb_numa_nodes; j++) {
>                  idx = (i * nb_numa_nodes + j) * 3;
> -                matrix[idx + 0] = cpu_to_be32(i);
> -                matrix[idx + 1] = cpu_to_be32(j);
> +                matrix[idx + 0] = 
> cpu_to_be32(ms->numa_state->nodes[i].nodeid);
> +                matrix[idx + 1] = 
> cpu_to_be32(ms->numa_state->nodes[j].nodeid);
>                  matrix[idx + 2] =
>                      cpu_to_be32(ms->numa_state->nodes[i].distance[j]);
>              }
> @@ -1845,7 +1845,7 @@ virt_cpu_index_to_props(MachineState *ms, unsigned 
> cpu_index)
>  
>  static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
>  {
> -    return idx % ms->numa_state->num_nodes;
> +    return ms->numa_state->nodes[idx % ms->numa_state->num_nodes].nodeid;
>  }
>  
>  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index b36d9a1ec8..faf6e05d84 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -643,11 +643,19 @@ void machine_set_cpu_numa_node(MachineState *machine,
>      NodeInfo *numa_info = machine->numa_state->nodes;
>      bool match = false;
>      int i;
> +    int node_id = find_numa(props->node_id, machine->numa_state);
>  
>      if (!mc->possible_cpu_arch_ids) {
>          error_setg(errp, "mapping of CPUs to NUMA node is not supported");
>          return;
>      }
> +    if (node_id == MAX_NODES) {
> +        if (props->has_node_id) {
> +            node_id = props->node_id;
> +        } else {
> +            node_id = machine->numa_state->num_nodes;
> +        }
> +    }
>  
>      /* disabling node mapping is not supported, forbid it */
>      assert(props->has_node_id);
> @@ -711,15 +719,15 @@ void machine_set_cpu_numa_node(MachineState *machine,
>          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)) {
> +        if (numa_info[node_id].initiator_valid &&
> +            (props->node_id != numa_info[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;
> +        numa_info[node_id].initiator_valid = true;
> +        numa_info[node_id].has_cpu = true;
> +        numa_info[node_id].initiator = props->node_id;
>      }
>  
>      if (!match) {
> @@ -1097,14 +1105,28 @@ static void machine_numa_finish_cpu_init(MachineState 
> *machine)
>      }
>  
>      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);
> +        int node_id;
> +        if (!numa_info[i].initiator_valid) {
> +            continue;
> +        }
> +        node_id = find_numa(numa_info[i].initiator, machine->numa_state);
> +        if (node_id == MAX_NODES) {
> +            error_report("The NUMA node %" PRIu16 " initiator node (id %" 
> PRIu16
> +                         ") does not exist", numa_info[i].nodeid,
> +                         numa_info[i].initiator);
> +            error_printf("\n");
> +
> +            exit(1);
> +        }
> +        if (!numa_info[node_id].has_cpu) {
> +            error_report("The NUMA node %" PRIu16 " initiator node (id %" 
> PRIu16
> +                         ") has no cpus", numa_info[i].nodeid,
> +                         numa_info[i].initiator);
>              error_printf("\n");
>  
>              exit(1);
>          }
> +
>      }
>  
>      if (s->len && !qtest_enabled()) {
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 75db35ac19..50a156f39f 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -48,9 +48,19 @@ QemuOptsList qemu_numa_opts = {
>  
>  static int have_memdevs;
>  static int have_mem;
> -static int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
> -                             * For all nodes, nodeid < max_numa_nodeid
> -                             */
> +
> +int find_numa(uint16_t node, NumaState *numa_state)
> +{
> +    NodeInfo *numa_info = numa_state->nodes;
> +    int nb_numa_nodes = numa_state->num_nodes;
> +
> +    for (int i = 0; i < nb_numa_nodes; i++) {
> +        if (numa_info[i].present && numa_info[i].nodeid == node) {
> +            return i;
> +        }
> +    }
> +    return MAX_NODES;
> +}
>  
>  static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>                              Error **errp)
> @@ -61,20 +71,18 @@ static void parse_numa_node(MachineState *ms, 
> NumaNodeOptions *node,
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
>      unsigned int max_cpus = ms->smp.max_cpus;
>      NodeInfo *numa_info = ms->numa_state->nodes;
> +    int nb_numa_nodes = ms->numa_state->num_nodes;
>  
> -    if (node->has_nodeid) {
> -        nodenr = node->nodeid;
> -    } else {
> -        nodenr = ms->numa_state->num_nodes;
> -    }
> +    nodenr = ms->numa_state->num_nodes;
>  
> -    if (nodenr >= MAX_NODES) {
> +    if (nb_numa_nodes >= MAX_NODES) {
>          error_setg(errp, "Max number of NUMA nodes reached: %"
>                     PRIu16 "", nodenr);
>          return;
>      }
>  
> -    if (numa_info[nodenr].present) {
> +    if (node->has_nodeid &&
> +        find_numa(node->nodeid, ms->numa_state) != MAX_NODES) {
>          error_setg(errp, "Duplicate NUMA nodeid: %" PRIu16, nodenr);
>          return;
>      }
> @@ -93,7 +101,7 @@ static void parse_numa_node(MachineState *ms, 
> NumaNodeOptions *node,
>              return;
>          }
>          props = mc->cpu_index_to_instance_props(ms, cpus->value);
> -        props.node_id = nodenr;
> +        props.node_id = node->has_nodeid ? node->nodeid : nodenr;
>          props.has_node_id = true;
>          machine_set_cpu_numa_node(ms, &props, &err);
>          if (err) {
> @@ -143,26 +151,26 @@ static void parse_numa_node(MachineState *ms, 
> NumaNodeOptions *node,
>          numa_info[nodenr].initiator = node->initiator;
>      }
>      numa_info[nodenr].present = true;
> -    max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
> +    numa_info[nodenr].nodeid = node->has_nodeid ? node->nodeid : 
> nb_numa_nodes;
>      ms->numa_state->num_nodes++;
>  }
>  
>  static
>  void parse_numa_distance(MachineState *ms, NumaDistOptions *dist, Error 
> **errp)
>  {
> -    uint16_t src = dist->src;
> -    uint16_t dst = dist->dst;
> +    int src = find_numa(dist->src, ms->numa_state);
> +    int dst = find_numa(dist->dst, ms->numa_state);
>      uint8_t val = dist->val;
>      NodeInfo *numa_info = ms->numa_state->nodes;
>  
> -    if (src >= MAX_NODES || dst >= MAX_NODES) {
> -        error_setg(errp, "Parameter '%s' expects an integer between 0 and 
> %d",
> -                   src >= MAX_NODES ? "src" : "dst", MAX_NODES - 1);
> +    if (src >= MAX_NODES || !numa_info[src].present) {
> +        error_setg(errp, "Source NUMA node is missing. "
> +                   "Please use '-numa node' option to declare it first.");
>          return;
>      }
>  
> -    if (!numa_info[src].present || !numa_info[dst].present) {
> -        error_setg(errp, "Source/Destination NUMA node is missing. "
> +    if (dst >= MAX_NODES || !numa_info[dst].present) {
> +        error_setg(errp, "Destination NUMA node is missing. "
>                     "Please use '-numa node' option to declare it first.");
>          return;
>      }
> @@ -175,8 +183,8 @@ void parse_numa_distance(MachineState *ms, 
> NumaDistOptions *dist, Error **errp)
>      }
>  
>      if (src == dst && val != NUMA_DISTANCE_MIN) {
> -        error_setg(errp, "Local distance of node %d should be %d.",
> -                   src, NUMA_DISTANCE_MIN);
> +        error_setg(errp, "Local distance of node %" PRIu16 " should be %d.",
> +                   dist->src, NUMA_DISTANCE_MIN);
>          return;
>      }
>  
> @@ -187,9 +195,10 @@ void parse_numa_distance(MachineState *ms, 
> NumaDistOptions *dist, Error **errp)
>  void parse_numa_hmat_lb(MachineState *ms, NumaHmatLBOptions *node,
>                          Error **errp)
>  {
> -    int nb_numa_nodes = ms->numa_state->num_nodes;
>      NodeInfo *numa_info = ms->numa_state->nodes;
>      HMAT_LB_Info *hmat_lb = NULL;
> +    int initiator = find_numa(node->initiator, ms->numa_state);
> +    int target = find_numa(node->target, ms->numa_state);
>  
>      if (node->data_type <= HMATLB_DATA_TYPE_WRITE_LATENCY) {
>          if (!node->has_latency) {
> @@ -225,26 +234,26 @@ void parse_numa_hmat_lb(MachineState *ms, 
> NumaHmatLBOptions *node,
>          }
>      }
>  
> -    if (node->initiator >= nb_numa_nodes) {
> +    if (initiator >= MAX_NODES) {
>          error_setg(errp, "Invalid initiator=%"
> -                   PRIu16 ", it should be less than %d.",
> -                   node->initiator, nb_numa_nodes);
> +                   PRIu16 ", not found.",
> +                   node->initiator);
>          return;
>      }
> -    if (!numa_info[node->initiator].has_cpu) {
> +    if (!numa_info[initiator].has_cpu) {
>          error_setg(errp, "Invalid initiator=%"
>                     PRIu16 ", it isn't an initiator proximity domain.",
>                     node->initiator);
>          return;
>      }
>  
> -    if (node->target >= nb_numa_nodes) {
> +    if (target >= MAX_NODES) {
>          error_setg(errp, "Invalid target=%"
> -                   PRIu16 ", it should be less than %d.",
> -                   node->target, nb_numa_nodes);
> +                   PRIu16 ", not found",
> +                   node->target);
>          return;
>      }
> -    if (!numa_info[node->target].initiator_valid) {
> +    if (!numa_info[target].initiator_valid) {
>          error_setg(errp, "Invalid target=%"
>                     PRIu16 ", it hasn't a valid initiator proximity domain.",
>                     node->target);
> @@ -257,7 +266,7 @@ void parse_numa_hmat_lb(MachineState *ms, 
> NumaHmatLBOptions *node,
>          if (!hmat_lb) {
>              hmat_lb = g_malloc0(sizeof(*hmat_lb));
>              ms->numa_state->hmat_lb[node->hierarchy][node->data_type] = 
> hmat_lb;
> -        } else if (hmat_lb->latency[node->initiator][node->target]) {
> +        } else if (hmat_lb->latency[initiator][target]) {
>              error_setg(errp, "Duplicate configuration of the latency for "
>                         "initiator=%" PRIu16 " and target=%" PRIu16 ".",
>                         node->initiator, node->target);
> @@ -269,7 +278,7 @@ void parse_numa_hmat_lb(MachineState *ms, 
> NumaHmatLBOptions *node,
>              hmat_lb->base_lat = node->base_lat;
>          }
>  
> -        hmat_lb->latency[node->initiator][node->target] = node->latency;
> +        hmat_lb->latency[initiator][target] = node->latency;
>      }
>  
>      if (node->has_bandwidth) {
> @@ -278,7 +287,7 @@ void parse_numa_hmat_lb(MachineState *ms, 
> NumaHmatLBOptions *node,
>          if (!hmat_lb) {
>              hmat_lb = g_malloc0(sizeof(*hmat_lb));
>              ms->numa_state->hmat_lb[node->hierarchy][node->data_type] = 
> hmat_lb;
> -        } else if (hmat_lb->bandwidth[node->initiator][node->target]) {
> +        } else if (hmat_lb->bandwidth[initiator][target]) {
>              error_setg(errp, "Duplicate configuration of the bandwidth for "
>                         "initiator=%" PRIu16 " and target=%" PRIu16 ".",
>                         node->initiator, node->target);
> @@ -295,7 +304,7 @@ void parse_numa_hmat_lb(MachineState *ms, 
> NumaHmatLBOptions *node,
>              }
>          }
>  
> -        hmat_lb->bandwidth[node->initiator][node->target] = node->bandwidth;
> +        hmat_lb->bandwidth[initiator][target] = node->bandwidth;
>      }
>  
>      if (hmat_lb) {
> @@ -307,13 +316,13 @@ void parse_numa_hmat_lb(MachineState *ms, 
> NumaHmatLBOptions *node,
>  void parse_numa_hmat_cache(MachineState *ms, NumaHmatCacheOptions *node,
>                             Error **errp)
>  {
> -    int nb_numa_nodes = ms->numa_state->num_nodes;
>      HMAT_Cache_Info *hmat_cache = NULL;
> +    int node_id = find_numa(node->node_id, ms->numa_state);
>  
> -    if (node->node_id >= nb_numa_nodes) {
> +    if (node_id >= MAX_NODES) {
>          error_setg(errp, "Invalid node-id=%" PRIu32
> -                   ", it should be less than %d.",
> -                   node->node_id, nb_numa_nodes);
> +                   ", not found.",
> +                   node->node_id);
>          return;
>      }
>  
> @@ -330,7 +339,7 @@ void parse_numa_hmat_cache(MachineState *ms, 
> NumaHmatCacheOptions *node,
>                     node->level, node->total);
>          return;
>      }
> -    if (ms->numa_state->hmat_cache[node->node_id][node->level]) {
> +    if (ms->numa_state->hmat_cache[node_id][node->level]) {
>          error_setg(errp, "Duplicate configuration of the side cache for "
>                     "node-id=%" PRIu32 " and level=%" PRIu8 ".",
>                     node->node_id, node->level);
> @@ -338,15 +347,15 @@ void parse_numa_hmat_cache(MachineState *ms, 
> NumaHmatCacheOptions *node,
>      }
>  
>      if ((node->level > 1) &&
> -        ms->numa_state->hmat_cache[node->node_id][node->level - 1] &&
> +        ms->numa_state->hmat_cache[node_id][node->level - 1] &&
>          (node->size >=
> -            ms->numa_state->hmat_cache[node->node_id][node->level - 
> 1]->size)) {
> +            ms->numa_state->hmat_cache[node_id][node->level - 1]->size)) {
>          error_setg(errp, "Invalid size=0x%" PRIx64
>                     ", the size of level=%" PRIu8
>                     " should be less than the size(0x%" PRIx64
>                     ") of level=%" PRIu8 ".",
>                     node->size, node->level,
> -                   ms->numa_state->hmat_cache[node->node_id]
> +                   ms->numa_state->hmat_cache[node_id]
>                                               [node->level - 1]->size,
>                     node->level - 1);
>          return;
> @@ -362,7 +371,7 @@ void parse_numa_hmat_cache(MachineState *ms, 
> NumaHmatCacheOptions *node,
>      hmat_cache->write_policy = node->policy;
>      hmat_cache->line_size = node->line;
>  
> -    ms->numa_state->hmat_cache[node->node_id][node->level] = hmat_cache;
> +    ms->numa_state->hmat_cache[node_id][node->level] = hmat_cache;
>  }
>  
>  void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
> @@ -393,7 +402,7 @@ void set_numa_options(MachineState *ms, NumaOptions 
> *object, Error **errp)
>              error_setg(&err, "Missing mandatory node-id property");
>              goto end;
>          }
> -        if (!ms->numa_state->nodes[object->u.cpu.node_id].present) {
> +        if (find_numa(object->u.cpu.node_id, ms->numa_state) == MAX_NODES) {
>              error_setg(&err, "Invalid node-id=%" PRId64 ", NUMA node must be 
> "
>                  "defined with -numa node,nodeid=ID before it's used with "
>                  "-numa cpu,node-id=ID", object->u.cpu.node_id);
> @@ -472,10 +481,11 @@ static void validate_numa_distance(MachineState *ms)
>              if (numa_info[src].distance[dst] == 0 &&
>                  numa_info[dst].distance[src] == 0) {
>                  if (src != dst) {
> -                    error_report("The distance between node %d and %d is "
> -                                 "missing, at least one distance value "
> -                                 "between each nodes should be provided.",
> -                                 src, dst);
> +                    error_report("The distance between node %" PRIu16
> +                                 " and %" PRIu16 " is missing, at least one "
> +                                 "distance value between each nodes should 
> be "
> +                                 "provided.",
> +                                 numa_info[src].nodeid, 
> numa_info[dst].nodeid);
>                      exit(EXIT_FAILURE);
>                  }
>              }
> @@ -493,9 +503,11 @@ static void validate_numa_distance(MachineState *ms)
>          for (src = 0; src < nb_numa_nodes; src++) {
>              for (dst = 0; dst < nb_numa_nodes; dst++) {
>                  if (src != dst && numa_info[src].distance[dst] == 0) {
> -                    error_report("At least one asymmetrical pair of "
> -                            "distances is given, please provide distances "
> -                            "for both directions of all node pairs.");
> +                    error_report("At least one asymmetrical pair (%" PRIu16
> +                            ", %" PRIu16 ") of distances is given, please "
> +                            "provide distances for both directions of all 
> node "
> +                            "pairs.",
> +                            numa_info[src].nodeid, numa_info[dst].nodeid);
>                      exit(EXIT_FAILURE);
>                  }
>              }
> @@ -587,27 +599,11 @@ void numa_complete_configuration(MachineState *ms)
>              parse_numa_node(ms, &node, &error_abort);
>      }
>  
> -    assert(max_numa_nodeid <= MAX_NODES);
> -
> -    /* No support for sparse NUMA node IDs yet: */
> -    for (i = max_numa_nodeid - 1; i >= 0; i--) {
> -        /* Report large node IDs first, to make mistakes easier to spot */
> -        if (!numa_info[i].present) {
> -            error_report("numa: Node ID missing: %d", i);
> -            exit(1);
> -        }
> -    }
> -
> -    /* This must be always true if all nodes are present: */
> -    assert(ms->numa_state->num_nodes == max_numa_nodeid);
> +    assert(ms->numa_state->num_nodes <= MAX_NODES);
>  
>      if (ms->numa_state->num_nodes > 0) {
>          uint64_t numa_total;
>  
> -        if (ms->numa_state->num_nodes > MAX_NODES) {
> -            ms->numa_state->num_nodes = MAX_NODES;
> -        }
> -
>          /* If no memory size is given for any node, assume the default case
>           * and distribute the available memory equally across all nodes
>           */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 90ad0dff99..f4a906c72e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2361,6 +2361,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>      numa_start = table_data->len;
>  
>      for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> +        int nodeid = machine->numa_state->nodes[i - 1].nodeid;
>          mem_base = next_base;
>          mem_len = pcms->node_mem[i - 1];
>          next_base = mem_base + mem_len;
> @@ -2371,7 +2372,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>              mem_len -= next_base - HOLE_640K_START;
>              if (mem_len > 0) {
>                  numamem = acpi_data_push(table_data, sizeof *numamem);
> -                build_srat_memory(numamem, mem_base, mem_len, i - 1,
> +                build_srat_memory(numamem, mem_base, mem_len, nodeid,
>                                    MEM_AFFINITY_ENABLED);
>              }
>  
> @@ -2390,7 +2391,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>              mem_len -= next_base - pcms->below_4g_mem_size;
>              if (mem_len > 0) {
>                  numamem = acpi_data_push(table_data, sizeof *numamem);
> -                build_srat_memory(numamem, mem_base, mem_len, i - 1,
> +                build_srat_memory(numamem, mem_base, mem_len, nodeid,
>                                    MEM_AFFINITY_ENABLED);
>              }
>              mem_base = 1ULL << 32;
> @@ -2400,7 +2401,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>  
>          if (mem_len > 0) {
>              numamem = acpi_data_push(table_data, sizeof *numamem);
> -            build_srat_memory(numamem, mem_base, mem_len, i - 1,
> +            build_srat_memory(numamem, mem_base, mem_len, nodeid,
>                                MEM_AFFINITY_ENABLED);
>          }
>      }
> @@ -2421,8 +2422,9 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>      if (hotplugabble_address_space_size) {
>          numamem = acpi_data_push(table_data, sizeof *numamem);
>          build_srat_memory(numamem, machine->device_memory->base,
> -                          hotplugabble_address_space_size, pcms->numa_nodes 
> - 1,
> -                          MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
> +            hotplugabble_address_space_size,
> +            machine->numa_state->nodes[pcms->numa_nodes - 1].nodeid,
> +            MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
>      }
>  
>      build_header(linker, table_data,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c3f5a70a56..5b8db454b7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2850,7 +2850,7 @@ static int64_t pc_get_default_cpu_node_id(const 
> MachineState *ms, int idx)
>     x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
>                              pcms->smp_dies, ms->smp.cores,
>                              ms->smp.threads, &topo);
> -   return topo.pkg_id % ms->numa_state->num_nodes;
> +   return ms->numa_state->nodes[topo.pkg_id % 
> ms->numa_state->num_nodes].nodeid;
>  }
>  
>  static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f607ca567b..ef4802698c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -424,7 +424,8 @@ static int spapr_populate_memory(SpaprMachineState 
> *spapr, void *fdt)
>          if (!mem_start) {
>              /* spapr_machine_init() checks for rma_size <= node0_size
>               * already */
> -            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> +            spapr_populate_memory_node(fdt, nodes[i].nodeid, 0,
> +                                       spapr->rma_size);
>              mem_start += spapr->rma_size;
>              node_size -= spapr->rma_size;
>          }
> @@ -436,7 +437,8 @@ static int spapr_populate_memory(SpaprMachineState 
> *spapr, void *fdt)
>                  sizetmp = 1ULL << ctzl(mem_start);
>              }
>  
> -            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
> +            spapr_populate_memory_node(fdt, nodes[i].nodeid, mem_start,
> +                                       sizetmp);
>              node_size -= sizetmp;
>              mem_start += sizetmp;
>          }
> @@ -2543,7 +2545,8 @@ static void spapr_validate_node_memory(MachineState 
> *machine, Error **errp)
>              error_setg(errp,
>                         "Node %d memory size 0x%" PRIx64
>                         " is not aligned to %" PRIu64 " MiB",
> -                       i, machine->numa_state->nodes[i].node_mem,
> +                       machine->numa_state->nodes[i].nodeid,
> +                       machine->numa_state->nodes[i].node_mem,
>                         SPAPR_MEMORY_BLOCK_SIZE / MiB);
>              return;
>          }
> @@ -4140,7 +4143,8 @@ spapr_cpu_index_to_props(MachineState *machine, 
> unsigned cpu_index)
>  
>  static int64_t spapr_get_default_cpu_node_id(const MachineState *ms, int idx)
>  {
> -    return idx / ms->smp.cores % ms->numa_state->num_nodes;
> +    return ms->numa_state->nodes[
> +        idx / ms->smp.cores % ms->numa_state->num_nodes].nodeid;
>  }
>  
>  static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState 
> *machine)
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 9009bbdee3..7474f2c5b6 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -13,6 +13,7 @@ struct NodeInfo {
>      bool has_cpu;
>      bool initiator_valid;
>      uint16_t initiator;
> +    uint16_t nodeid;
>      uint8_t distance[MAX_NODES];
>  };
>  
> @@ -39,6 +40,7 @@ struct NumaState {
>  };
>  typedef struct NumaState NumaState;
>  
> +int find_numa(uint16_t node, NumaState *numa_state);
>  void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp);
>  void parse_numa_opts(MachineState *ms);
>  void parse_numa_hmat_lb(MachineState *ms, NumaHmatLBOptions *node,

-- 
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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]