On Wed, Mar 22, 2017 at 11:34:05AM +0100, Andrew Jones wrote: > > You should have CC'ed me, considering this version is addressing my > review comments, but whatever, I usually skim the list so I found it... > > On Wed, Mar 22, 2017 at 05:32:46PM +0800, He Chen wrote: > > Current, QEMU does not provide a clear command to set vNUMA distance for > > guest although we already have `-numa` command to set vNUMA nodes. > > > > vNUMA distance makes sense in certain scenario. > > But now, if we create a guest that has 4 vNUMA nodes, when we check NUMA > > info via `numactl -H`, we will see: > > > > node distance: > > node 0 1 2 3 > > 0: 10 20 20 20 > > 1: 20 10 20 20 > > 2: 20 20 10 20 > > 3: 20 20 20 10 > > > > Guest kernel regards all local node as distance 10, and all remote node > > as distance 20 when there is no SLIT table since QEMU doesn't build it. > > It looks like a little strange when you have seen the distance in an > > actual physical machine that contains 4 NUMA nodes. My machine shows: > > > > node distance: > > node 0 1 2 3 > > 0: 10 21 31 41 > > 1: 21 10 21 31 > > 2: 31 21 10 21 > > 3: 41 31 21 10 > > > > To set vNUMA distance, guest should see a complete SLIT table. > > I found QEMU has provide `-acpitable` command that allows users to add > > a ACPI table into guest, but it requires users building ACPI table by > > themselves first. Using `-acpitable` to add a SLIT table may be not so > > straightforward or flexible, imagine that when the vNUMA configuration > > is changes and we need to generate another SLIT table manually. It may > > not be friendly to users or upper software like libvirt. > > > > This patch is going to add SLIT table support in QEMU, and provides > > additional option `dist` for command `-numa` to allow user set vNUMA > > distance by QEMU command. > > > > With this patch, when a user wants to create a guest that contains > > several vNUMA nodes and also wants to set distance among those nodes, > > the QEMU command would like: > > > > ``` > > -object > > memory-backend-ram,size=1G,prealloc=yes,host-nodes=0,policy=bind,id=node0 \ > > -numa node,nodeid=0,cpus=0,memdev=node0 \ > > -object > > memory-backend-ram,size=1G,prealloc=yes,host-nodes=1,policy=bind,id=node1 \ > > -numa node,nodeid=1,cpus=1,memdev=node1 \ > > -object > > memory-backend-ram,size=1G,prealloc=yes,host-nodes=2,policy=bind,id=node2 \ > > -numa node,nodeid=2,cpus=2,memdev=node2 \ > > -object > > memory-backend-ram,size=1G,prealloc=yes,host-nodes=3,policy=bind,id=node3 \ > > -numa node,nodeid=3,cpus=3,memdev=node3 \ > > -numa dist,src=0,dst=1,val=21 \ > > -numa dist,src=0,dst=2,val=31 \ > > -numa dist,src=0,dst=3,val=41 \ > > -numa dist,src=1,dst=0,val=21 \ > > ... > > ``` > > > > Signed-off-by: He Chen <he.c...@linux.intel.com> > > > > fix > > stray 'fix' above the --- > > > --- > > hw/acpi/aml-build.c | 26 +++++++++++++++++++++++++ > > hw/arm/virt-acpi-build.c | 2 ++ > > hw/i386/acpi-build.c | 2 ++ > > include/hw/acpi/aml-build.h | 1 + > > include/sysemu/numa.h | 1 + > > include/sysemu/sysemu.h | 4 ++++ > > numa.c | 47 > > +++++++++++++++++++++++++++++++++++++++++++++ > > qapi-schema.json | 30 ++++++++++++++++++++++++++--- > > qemu-options.hx | 12 +++++++++++- > > 9 files changed, 121 insertions(+), 4 deletions(-) > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > index c6f2032..410b30e 100644 > > --- a/hw/acpi/aml-build.c > > +++ b/hw/acpi/aml-build.c > > @@ -24,6 +24,7 @@ > > #include "hw/acpi/aml-build.h" > > #include "qemu/bswap.h" > > #include "qemu/bitops.h" > > +#include "sysemu/numa.h" > > > > static GArray *build_alloc_array(void) > > { > > @@ -1609,3 +1610,28 @@ void build_srat_memory(AcpiSratMemoryAffinity > > *numamem, uint64_t base, > > numamem->base_addr = cpu_to_le64(base); > > numamem->range_length = cpu_to_le64(len); > > } > > + > > +/* > > + * ACPI spec 5.2.17 System Locality Distance Information Table > > + * (Revision 2.0 or later) > > + */ > > +void build_slit(GArray *table_data, BIOSLinker *linker) > > +{ > > + int slit_start, i, j; > > + slit_start = table_data->len; > > + > > + 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++) { > > + build_append_int_noprefix(table_data, > > numa_info[i].distance[j], 1); > > + } > > + } > > + > > + build_header(linker, table_data, > > + (void *)(table_data->data + slit_start), > > + "SLIT", > > + table_data->len - slit_start, 1, NULL, NULL); > > +} > > + > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index 0835e59..d9e6828 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -781,6 +781,8 @@ void virt_acpi_build(VirtMachineState *vms, > > AcpiBuildTables *tables) > > if (nb_numa_nodes > 0) { > > acpi_add_table(table_offsets, tables_blob); > > build_srat(tables_blob, tables->linker, vms); > > + acpi_add_table(table_offsets, tables_blob); > > + build_slit(tables_blob, tables->linker); > > I didn't expect you'd actually add the support to the ARM machine > model, but only provide the API. Anyway, thanks for this, assuming > you've tested it and it works. > > > }
Well... I am sorry that actually I did not test it since I don't have ARM platform. I am afraid I will remove it in next patch. > > > > if (its_class_name() && !vmc->no_its) { > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 2073108..12730ea 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -2678,6 +2678,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState > > *machine) > > if (pcms->numa_nodes) { > > acpi_add_table(table_offsets, tables_blob); > > build_srat(tables_blob, tables->linker, machine); > > + acpi_add_table(table_offsets, tables_blob); > > + build_slit(tables_blob, tables->linker); > > } > > if (acpi_get_mcfg(&mcfg)) { > > acpi_add_table(table_offsets, tables_blob); > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > > index 00c21f1..329a0d0 100644 > > --- a/include/hw/acpi/aml-build.h > > +++ b/include/hw/acpi/aml-build.h > > @@ -389,4 +389,5 @@ GCC_FMT_ATTR(2, 3); > > void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, > > uint64_t len, int node, MemoryAffinityFlags flags); > > > > +void build_slit(GArray *table_data, BIOSLinker *linker); > > #endif > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h > > index 8f09dcf..2f7a941 100644 > > --- a/include/sysemu/numa.h > > +++ b/include/sysemu/numa.h > > @@ -21,6 +21,7 @@ typedef struct node_info { > > struct HostMemoryBackend *node_memdev; > > bool present; > > QLIST_HEAD(, numa_addr_range) addr; /* List to store address ranges */ > > + uint8_t distance[MAX_NODES]; > > } NodeInfo; > > > > extern NodeInfo numa_info[MAX_NODES]; > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > > index 576c7ce..a4e328d 100644 > > --- a/include/sysemu/sysemu.h > > +++ b/include/sysemu/sysemu.h > > @@ -169,6 +169,10 @@ extern int mem_prealloc; > > > > #define MAX_NODES 128 > > #define NUMA_NODE_UNASSIGNED MAX_NODES > > +#define MIN_NUMA_DISTANCE 10 > > +#define DEF_NUMA_DISTANCE 20 > > +#define MAX_NUMA_DISTANCE 254 > > You ignored my comment about starting with 'NUMA_DISTANCE' without > even replying to that comment stating why you prefer it this way. > That's a bit annoying. > Apologyies for my carelessness. I miss this comment, and surely, I will change these names in next patch. > > +#define NUMA_DISTANCE_UNREACHABLE 255 > > Here you do start with NUMA_DISTANCE, so these names have an > inconsistent format. > > > > > #define MAX_OPTION_ROMS 16 > > typedef struct QEMUOptionRom { > > diff --git a/numa.c b/numa.c > > index e01cb54..425a320 100644 > > --- a/numa.c > > +++ b/numa.c > > @@ -212,6 +212,28 @@ static void numa_node_parse(NumaNodeOptions *node, > > QemuOpts *opts, Error **errp) > > max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1); > > } > > > > +static void numa_distance_parse(NumaDistOptions *dist, QemuOpts *opts, > > Error **errp) > > +{ > > + uint64_t src = dist->src; > > + uint64_t dst = dist->dst; > > + uint8_t val = dist->val; > > + > > + if (src >= MAX_NODES || dst >= MAX_NODES) { > > + error_setg(errp, "Max number of NUMA nodes reached: %" > > + PRIu64 "", src > dst ? src : dst); > > + return; > > + } > > + > > + if (val < MIN_NUMA_DISTANCE || val > MAX_NUMA_DISTANCE) { > > val > MAX_NUMA_DISTANCE is not invalid, as NUMA_DISTANCE_UNREACHABLE > is 255. Isn't it possible that that could be what the user wants? > Yes, you're right. > > + error_setg(errp, > > + "NUMA distance (%" PRIu8 ") out of range (%d) ~ (%d)", > > + dist->val, MIN_NUMA_DISTANCE, MAX_NUMA_DISTANCE); > > + return; > > + } > > + > > + numa_info[src].distance[dst] = val; > > +} > > + > > static int parse_numa(void *opaque, QemuOpts *opts, Error **errp) > > { > > NumaOptions *object = NULL; > > @@ -235,6 +257,12 @@ static int parse_numa(void *opaque, QemuOpts *opts, > > Error **errp) > > } > > nb_numa_nodes++; > > break; > > + case NUMA_OPTIONS_TYPE_DIST: > > + numa_distance_parse(&object->u.dist, opts, &err); > > + if (err) { > > + goto end; > > + } > > + break; > > default: > > abort(); > > } > > @@ -294,6 +322,24 @@ static void validate_numa_cpus(void) > > g_free(seen_cpus); > > } > > > > +static void default_numa_distance(void) > > +{ > > + int src, dst; > > + > > + for (src = 0; src < nb_numa_nodes; src++) { > > + for (dst = 0; dst < nb_numa_nodes; dst++) { > > + if (src == dst && numa_info[src].distance[dst] != > > MIN_NUMA_DISTANCE) { > > + numa_info[src].distance[dst] = MIN_NUMA_DISTANCE; > > + } else if (numa_info[src].distance[dst] <= MIN_NUMA_DISTANCE) { > > + if (numa_info[dst].distance[src] > MIN_NUMA_DISTANCE) > > + numa_info[src].distance[dst] = > > numa_info[dst].distance[src]; > > + else > > + numa_info[src].distance[dst] = DEF_NUMA_DISTANCE; > > + } > > + } > > + } > > +} > > + > > void parse_numa_opts(MachineClass *mc) > > { > > int i; > > @@ -390,6 +436,7 @@ void parse_numa_opts(MachineClass *mc) > > } > > > > validate_numa_cpus(); > > + default_numa_distance(); > > } else { > > numa_set_mem_node_id(0, ram_size, 0); > > } > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 32b4a4b..21ad94a 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -5644,15 +5644,19 @@ > > ## > > # @NumaOptionsType: > > # > > +# @node: NUMA nodes configuration > > +# > > +# @dist: NUMA distance configuration > > +# > > # Since: 2.1 > > ## > > { 'enum': 'NumaOptionsType', > > - 'data': [ 'node' ] } > > + 'data': [ 'node', 'dist' ] } > > > > ## > > # @NumaOptions: > > # > > -# A discriminated record of NUMA options. (for OptsVisitor) > > +# A discriminated record of NUMA options. > > # > > # Since: 2.1 > > ## > > @@ -5660,7 +5664,8 @@ > > 'base': { 'type': 'NumaOptionsType' }, > > 'discriminator': 'type', > > 'data': { > > - 'node': 'NumaNodeOptions' }} > > + 'node': 'NumaNodeOptions', > > + 'dist': 'NumaDistOptions' }} > > > > ## > > # @NumaNodeOptions: > > @@ -5689,6 +5694,25 @@ > > '*memdev': 'str' }} > > > > ## > > +# @NumaDistOptions: > > +# > > +# Set distance between 2 NUMA nodes. (for OptsVisitor) > > Set the distance > > > +# > > +# @src: source NUMA node. > > +# > > +# @dst: destination NUMA node. > > +# > > +# @val: NUMA distance from source node to destination node. > > +# > > +# Since: 2.10 > > +## > > +{ 'struct': 'NumaDistOptions', > > + 'data': { > > + 'src': 'uint64', > > + 'dst': 'uint64', > > + 'val': 'uint8' }} > > + > > +## > > # @HostMemPolicy: > > # > > # Host memory policy types > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 8dd8ee3..43c3950 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -139,12 +139,15 @@ 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", > > QEMU_ARCH_ALL) > > + "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node]\n" > > + "-numa dist,src=source,dst=destination,val=distance\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}] > > +@itemx -numa dist,src=@var{source},dst=@var{destination},val=@var{distance} > > @findex -numa > > Define a NUMA node and assign RAM and VCPUs to it. > > +Set NUMA distance from source node to destination node. > > Set the NUMA distance from a source node to a destination node. > > > > > @var{firstcpu} and @var{lastcpu} are CPU indexes. Each > > @samp{cpus} option represent a contiguous range of CPU indexes > > @@ -167,6 +170,13 @@ 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. > > > > +@var{source} and @var{destination} are NUMA node IDs. > > +@var{distance} is the NUMA distance from @var{source} to @var{destination}. > > +The distance from node A to node B may be different from the distance from > > +node B to node A since the distance can to be asymmetry. > > /since/as/ > /asymmetry/asymmetrical/ > > > +If the distance is not set, the default distance for a local NUMA node is > > 10, > > no need for , > > > +and 20 for a remote node. > > and the default distance for a remote node is 20. > > Also need sentence like: When the distance for only one direction is > given, then the opposite direction defaults to the same distance. > > And maybe should also point out how to set an unreachable node with 255. > Thanks for your careful review. Regarding setting unreachable node, which way is good? Set exact 255 as distance or distance that is larger than 255 is regarded as unreachable? > > + > > Note that the -@option{numa} option doesn't allocate any of the > > specified resources, it just assigns existing resources to NUMA > > nodes. This means that one still has to use the @option{-m}, > > -- > > 2.7.4 > > > > > > drew