* [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
@ 2017-08-16 1:45 Dou Liyang
2017-08-17 5:58 ` Dou Liyang
2017-08-18 18:35 ` Michael S. Tsirkin
0 siblings, 2 replies; 9+ messages in thread
From: Dou Liyang @ 2017-08-16 1:45 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, rth, ehabkost, imammedo, mst, Dou Liyang
Currently, Using the fisrt node without memory on the machine makes
QEMU unhappy. With this example command line:
... \
-m 1024M,slots=4,maxmem=32G \
-numa node,nodeid=0 \
-numa node,mem=1024M,nodeid=1 \
-numa node,nodeid=2 \
-numa node,nodeid=3 \
Guest reports "No NUMA configuration found" and the NUMA topology is
wrong.
This is because when QEMU builds ACPI SRAT, it regards node0 as the
default node to deal with the memory hole(640K-1M). this means the
node0 must have some memory(>1M), but, actually it can have no
memory.
Fix this problem by replace the node0 with the first node which has
memory on it. Add a new function for each node. Also do some cleanup.
Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
V3 --> V2
-Modify the title
V2 --> V1:
-Fix a coding style problem
Replace
for (node = 0;
node < pcms->numa_nodes && pcms->node_mem[node] == 0;
node++);
with
for (node = 0; node < pcms->numa_nodes; node++) {
if (pcms->node_mem[node] != 0) {
break;
}
hw/i386/acpi-build.c | 78 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 50 insertions(+), 28 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 98dd424..f93d712 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
(void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
}
+static uint64_t
+build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
+ int i, uint64_t mem_base, uint64_t mem_len)
+{
+ AcpiSratMemoryAffinity *numamem;
+ uint64_t next_base;
+
+ next_base = mem_base + mem_len;
+
+ /* Cut out the ACPI_PCI hole */
+ if (mem_base <= pcms->below_4g_mem_size &&
+ next_base > pcms->below_4g_mem_size) {
+ 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,
+ MEM_AFFINITY_ENABLED);
+ }
+ mem_base = 1ULL << 32;
+ mem_len = next_base - pcms->below_4g_mem_size;
+ next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+ }
+ numamem = acpi_data_push(table_data, sizeof *numamem);
+ build_srat_memory(numamem, mem_base, mem_len, i,
+ MEM_AFFINITY_ENABLED);
+ return next_base;
+}
+
static void
build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
{
AcpiSystemResourceAffinityTable *srat;
AcpiSratMemoryAffinity *numamem;
- int i;
+ int i, node;
int srat_start, numa_start, slots;
- uint64_t mem_len, mem_base, next_base;
+ uint64_t mem_len, mem_base;
MachineClass *mc = MACHINE_GET_CLASS(machine);
const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
PCMachineState *pcms = PC_MACHINE(machine);
@@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
/* the memory map is a bit tricky, it contains at least one hole
* from 640k-1M and possibly another one from 3.5G-4G.
*/
- next_base = 0;
+
numa_start = table_data->len;
- numamem = acpi_data_push(table_data, sizeof *numamem);
- build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
- next_base = 1024 * 1024;
- for (i = 1; i < pcms->numa_nodes + 1; ++i) {
- mem_base = next_base;
- mem_len = pcms->node_mem[i - 1];
- if (i == 1) {
- mem_len -= 1024 * 1024;
+ /* get the first node which has memory and map the hole from 640K-1M */
+ for (node = 0; node < pcms->numa_nodes; node++) {
+ if (pcms->node_mem[node] != 0) {
+ break;
}
- next_base = mem_base + mem_len;
-
- /* Cut out the ACPI_PCI hole */
- if (mem_base <= pcms->below_4g_mem_size &&
- next_base > pcms->below_4g_mem_size) {
- 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,
- MEM_AFFINITY_ENABLED);
- }
- mem_base = 1ULL << 32;
- mem_len = next_base - pcms->below_4g_mem_size;
- next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+ }
+ numamem = acpi_data_push(table_data, sizeof *numamem);
+ build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
+
+ /* map the rest of memory from 1M */
+ mem_base = 1024 * 1024;
+ mem_len = pcms->node_mem[node] - mem_base;
+ mem_base = build_srat_node_entry(table_data, pcms, node,
+ mem_base, mem_len);
+
+ for (i = 0; i < pcms->numa_nodes; i++) {
+ if (i == node) {
+ continue;
}
- numamem = acpi_data_push(table_data, sizeof *numamem);
- build_srat_memory(numamem, mem_base, mem_len, i - 1,
- MEM_AFFINITY_ENABLED);
+ mem_base = build_srat_node_entry(table_data, pcms, i,
+ mem_base, pcms->node_mem[i]);
}
slots = (table_data->len - numa_start) / sizeof *numamem;
for (; slots < pcms->numa_nodes + 2; slots++) {
--
2.5.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-16 1:45 [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0 Dou Liyang
@ 2017-08-17 5:58 ` Dou Liyang
2017-08-18 16:48 ` Eduardo Habkost
2017-08-18 18:35 ` Michael S. Tsirkin
1 sibling, 1 reply; 9+ messages in thread
From: Dou Liyang @ 2017-08-17 5:58 UTC (permalink / raw)
To: qemu-devel, imammedo; +Cc: pbonzini, rth, ehabkost, mst
Hi Igor,
I tested this patch with following guests:
1. RHEL 6.5 with Linux 2.6.32
2. RHEL 7.0 with Linux 3.10.0
3. Fedora 23 with Linux 4.13.0-rc5
4. window 2003 service
5. window 7
6. window 10
Thanks,
dou.
At 08/16/2017 09:45 AM, Dou Liyang wrote:
> Currently, Using the fisrt node without memory on the machine makes
> QEMU unhappy. With this example command line:
> ... \
> -m 1024M,slots=4,maxmem=32G \
> -numa node,nodeid=0 \
> -numa node,mem=1024M,nodeid=1 \
> -numa node,nodeid=2 \
> -numa node,nodeid=3 \
> Guest reports "No NUMA configuration found" and the NUMA topology is
> wrong.
>
> This is because when QEMU builds ACPI SRAT, it regards node0 as the
> default node to deal with the memory hole(640K-1M). this means the
> node0 must have some memory(>1M), but, actually it can have no
> memory.
>
> Fix this problem by replace the node0 with the first node which has
> memory on it. Add a new function for each node. Also do some cleanup.
>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
> V3 --> V2
> -Modify the title
> V2 --> V1:
> -Fix a coding style problem
> Replace
> for (node = 0;
> node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> node++);
>
> with
> for (node = 0; node < pcms->numa_nodes; node++) {
> if (pcms->node_mem[node] != 0) {
> break;
> }
>
> hw/i386/acpi-build.c | 78 +++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..f93d712 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> }
>
> +static uint64_t
> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> + int i, uint64_t mem_base, uint64_t mem_len)
> +{
> + AcpiSratMemoryAffinity *numamem;
> + uint64_t next_base;
> +
> + next_base = mem_base + mem_len;
> +
> + /* Cut out the ACPI_PCI hole */
> + if (mem_base <= pcms->below_4g_mem_size &&
> + next_base > pcms->below_4g_mem_size) {
> + 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,
> + MEM_AFFINITY_ENABLED);
> + }
> + mem_base = 1ULL << 32;
> + mem_len = next_base - pcms->below_4g_mem_size;
> + next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> + }
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, mem_base, mem_len, i,
> + MEM_AFFINITY_ENABLED);
> + return next_base;
> +}
> +
> static void
> build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> {
> AcpiSystemResourceAffinityTable *srat;
> AcpiSratMemoryAffinity *numamem;
>
> - int i;
> + int i, node;
> int srat_start, numa_start, slots;
> - uint64_t mem_len, mem_base, next_base;
> + uint64_t mem_len, mem_base;
> MachineClass *mc = MACHINE_GET_CLASS(machine);
> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> PCMachineState *pcms = PC_MACHINE(machine);
> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> /* the memory map is a bit tricky, it contains at least one hole
> * from 640k-1M and possibly another one from 3.5G-4G.
> */
> - next_base = 0;
> +
> numa_start = table_data->len;
>
> - numamem = acpi_data_push(table_data, sizeof *numamem);
> - build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> - next_base = 1024 * 1024;
> - for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> - mem_base = next_base;
> - mem_len = pcms->node_mem[i - 1];
> - if (i == 1) {
> - mem_len -= 1024 * 1024;
> + /* get the first node which has memory and map the hole from 640K-1M */
> + for (node = 0; node < pcms->numa_nodes; node++) {
> + if (pcms->node_mem[node] != 0) {
> + break;
> }
> - next_base = mem_base + mem_len;
> -
> - /* Cut out the ACPI_PCI hole */
> - if (mem_base <= pcms->below_4g_mem_size &&
> - next_base > pcms->below_4g_mem_size) {
> - 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,
> - MEM_AFFINITY_ENABLED);
> - }
> - mem_base = 1ULL << 32;
> - mem_len = next_base - pcms->below_4g_mem_size;
> - next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> + }
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
> +
> + /* map the rest of memory from 1M */
> + mem_base = 1024 * 1024;
> + mem_len = pcms->node_mem[node] - mem_base;
> + mem_base = build_srat_node_entry(table_data, pcms, node,
> + mem_base, mem_len);
> +
> + for (i = 0; i < pcms->numa_nodes; i++) {
> + if (i == node) {
> + continue;
> }
> - numamem = acpi_data_push(table_data, sizeof *numamem);
> - build_srat_memory(numamem, mem_base, mem_len, i - 1,
> - MEM_AFFINITY_ENABLED);
> + mem_base = build_srat_node_entry(table_data, pcms, i,
> + mem_base, pcms->node_mem[i]);
> }
> slots = (table_data->len - numa_start) / sizeof *numamem;
> for (; slots < pcms->numa_nodes + 2; slots++) {
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-17 5:58 ` Dou Liyang
@ 2017-08-18 16:48 ` Eduardo Habkost
2017-08-20 11:52 ` Dou Liyang
0 siblings, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2017-08-18 16:48 UTC (permalink / raw)
To: Dou Liyang; +Cc: qemu-devel, imammedo, pbonzini, rth, mst
On Thu, Aug 17, 2017 at 01:58:40PM +0800, Dou Liyang wrote:
> Hi Igor,
>
> I tested this patch with following guests:
>
> 1. RHEL 6.5 with Linux 2.6.32
> 2. RHEL 7.0 with Linux 3.10.0
> 3. Fedora 23 with Linux 4.13.0-rc5
> 4. window 2003 service
> 5. window 7
> 6. window 10
What's the command-line(s) you have tested with each OS? Have
you tested both the node0-with-RAM and node0-without-RAM cases?
I would be interested in a demonstration that no bytes in the
ACPI table are changed by this patch when there is some RAM
configured in node 0. (Is that already covered by our existing
ACPI test cases?)
A new test case in bios-tables-test.c for the bug you are fixing
would be nice to have.
>
> Thanks,
> dou.
>
> At 08/16/2017 09:45 AM, Dou Liyang wrote:
> > Currently, Using the fisrt node without memory on the machine makes
> > QEMU unhappy. With this example command line:
> > ... \
> > -m 1024M,slots=4,maxmem=32G \
> > -numa node,nodeid=0 \
> > -numa node,mem=1024M,nodeid=1 \
> > -numa node,nodeid=2 \
> > -numa node,nodeid=3 \
> > Guest reports "No NUMA configuration found" and the NUMA topology is
> > wrong.
> >
> > This is because when QEMU builds ACPI SRAT, it regards node0 as the
> > default node to deal with the memory hole(640K-1M). this means the
> > node0 must have some memory(>1M), but, actually it can have no
> > memory.
> >
> > Fix this problem by replace the node0 with the first node which has
> > memory on it. Add a new function for each node. Also do some cleanup.
> >
> > Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> > ---
> > V3 --> V2
> > -Modify the title
> > V2 --> V1:
> > -Fix a coding style problem
> > Replace
> > for (node = 0;
> > node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> > node++);
> >
> > with
> > for (node = 0; node < pcms->numa_nodes; node++) {
> > if (pcms->node_mem[node] != 0) {
> > break;
> > }
> >
> > hw/i386/acpi-build.c | 78 +++++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 50 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 98dd424..f93d712 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> > (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> > }
> >
> > +static uint64_t
> > +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> > + int i, uint64_t mem_base, uint64_t mem_len)
> > +{
> > + AcpiSratMemoryAffinity *numamem;
> > + uint64_t next_base;
> > +
> > + next_base = mem_base + mem_len;
> > +
> > + /* Cut out the ACPI_PCI hole */
> > + if (mem_base <= pcms->below_4g_mem_size &&
> > + next_base > pcms->below_4g_mem_size) {
> > + 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,
> > + MEM_AFFINITY_ENABLED);
> > + }
> > + mem_base = 1ULL << 32;
> > + mem_len = next_base - pcms->below_4g_mem_size;
> > + next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> > + }
> > + numamem = acpi_data_push(table_data, sizeof *numamem);
> > + build_srat_memory(numamem, mem_base, mem_len, i,
> > + MEM_AFFINITY_ENABLED);
> > + return next_base;
> > +}
> > +
> > static void
> > build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> > {
> > AcpiSystemResourceAffinityTable *srat;
> > AcpiSratMemoryAffinity *numamem;
> >
> > - int i;
> > + int i, node;
> > int srat_start, numa_start, slots;
> > - uint64_t mem_len, mem_base, next_base;
> > + uint64_t mem_len, mem_base;
> > MachineClass *mc = MACHINE_GET_CLASS(machine);
> > const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> > PCMachineState *pcms = PC_MACHINE(machine);
> > @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> > /* the memory map is a bit tricky, it contains at least one hole
> > * from 640k-1M and possibly another one from 3.5G-4G.
> > */
> > - next_base = 0;
> > +
> > numa_start = table_data->len;
> >
> > - numamem = acpi_data_push(table_data, sizeof *numamem);
> > - build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> > - next_base = 1024 * 1024;
> > - for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> > - mem_base = next_base;
> > - mem_len = pcms->node_mem[i - 1];
> > - if (i == 1) {
> > - mem_len -= 1024 * 1024;
> > + /* get the first node which has memory and map the hole from 640K-1M */
> > + for (node = 0; node < pcms->numa_nodes; node++) {
> > + if (pcms->node_mem[node] != 0) {
> > + break;
> > }
> > - next_base = mem_base + mem_len;
> > -
> > - /* Cut out the ACPI_PCI hole */
> > - if (mem_base <= pcms->below_4g_mem_size &&
> > - next_base > pcms->below_4g_mem_size) {
> > - 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,
> > - MEM_AFFINITY_ENABLED);
> > - }
> > - mem_base = 1ULL << 32;
> > - mem_len = next_base - pcms->below_4g_mem_size;
> > - next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> > + }
> > + numamem = acpi_data_push(table_data, sizeof *numamem);
> > + build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
> > +
> > + /* map the rest of memory from 1M */
> > + mem_base = 1024 * 1024;
> > + mem_len = pcms->node_mem[node] - mem_base;
> > + mem_base = build_srat_node_entry(table_data, pcms, node,
> > + mem_base, mem_len);
> > +
> > + for (i = 0; i < pcms->numa_nodes; i++) {
> > + if (i == node) {
> > + continue;
> > }
> > - numamem = acpi_data_push(table_data, sizeof *numamem);
> > - build_srat_memory(numamem, mem_base, mem_len, i - 1,
> > - MEM_AFFINITY_ENABLED);
> > + mem_base = build_srat_node_entry(table_data, pcms, i,
> > + mem_base, pcms->node_mem[i]);
> > }
> > slots = (table_data->len - numa_start) / sizeof *numamem;
> > for (; slots < pcms->numa_nodes + 2; slots++) {
> >
>
>
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-16 1:45 [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0 Dou Liyang
2017-08-17 5:58 ` Dou Liyang
@ 2017-08-18 18:35 ` Michael S. Tsirkin
2017-08-18 19:20 ` Eduardo Habkost
1 sibling, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2017-08-18 18:35 UTC (permalink / raw)
To: Dou Liyang; +Cc: qemu-devel, pbonzini, rth, ehabkost, imammedo
On Wed, Aug 16, 2017 at 09:45:35AM +0800, Dou Liyang wrote:
> Currently, Using the fisrt node without memory on the machine makes
> QEMU unhappy. With this example command line:
> ... \
> -m 1024M,slots=4,maxmem=32G \
> -numa node,nodeid=0 \
> -numa node,mem=1024M,nodeid=1 \
> -numa node,nodeid=2 \
> -numa node,nodeid=3 \
> Guest reports "No NUMA configuration found" and the NUMA topology is
> wrong.
>
> This is because when QEMU builds ACPI SRAT, it regards node0 as the
> default node to deal with the memory hole(640K-1M). this means the
> node0 must have some memory(>1M), but, actually it can have no
> memory.
>
> Fix this problem by replace the node0 with the first node which has
> memory on it. Add a new function for each node. Also do some cleanup.
>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
This isn't a regression, is it? If it isn't, it's not a 2.10 candidate
IMHO.
> ---
> V3 --> V2
> -Modify the title
> V2 --> V1:
> -Fix a coding style problem
> Replace
> for (node = 0;
> node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> node++);
>
> with
> for (node = 0; node < pcms->numa_nodes; node++) {
> if (pcms->node_mem[node] != 0) {
> break;
> }
>
> hw/i386/acpi-build.c | 78 +++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..f93d712 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> }
>
> +static uint64_t
> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> + int i, uint64_t mem_base, uint64_t mem_len)
> +{
> + AcpiSratMemoryAffinity *numamem;
> + uint64_t next_base;
> +
> + next_base = mem_base + mem_len;
> +
> + /* Cut out the ACPI_PCI hole */
> + if (mem_base <= pcms->below_4g_mem_size &&
> + next_base > pcms->below_4g_mem_size) {
> + 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,
> + MEM_AFFINITY_ENABLED);
> + }
> + mem_base = 1ULL << 32;
> + mem_len = next_base - pcms->below_4g_mem_size;
> + next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> + }
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, mem_base, mem_len, i,
> + MEM_AFFINITY_ENABLED);
> + return next_base;
> +}
> +
> static void
> build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> {
> AcpiSystemResourceAffinityTable *srat;
> AcpiSratMemoryAffinity *numamem;
>
> - int i;
> + int i, node;
> int srat_start, numa_start, slots;
> - uint64_t mem_len, mem_base, next_base;
> + uint64_t mem_len, mem_base;
> MachineClass *mc = MACHINE_GET_CLASS(machine);
> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> PCMachineState *pcms = PC_MACHINE(machine);
> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> /* the memory map is a bit tricky, it contains at least one hole
> * from 640k-1M and possibly another one from 3.5G-4G.
> */
> - next_base = 0;
> +
> numa_start = table_data->len;
>
> - numamem = acpi_data_push(table_data, sizeof *numamem);
> - build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> - next_base = 1024 * 1024;
> - for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> - mem_base = next_base;
> - mem_len = pcms->node_mem[i - 1];
> - if (i == 1) {
> - mem_len -= 1024 * 1024;
> + /* get the first node which has memory and map the hole from 640K-1M */
> + for (node = 0; node < pcms->numa_nodes; node++) {
> + if (pcms->node_mem[node] != 0) {
> + break;
> }
> - next_base = mem_base + mem_len;
> -
> - /* Cut out the ACPI_PCI hole */
> - if (mem_base <= pcms->below_4g_mem_size &&
> - next_base > pcms->below_4g_mem_size) {
> - 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,
> - MEM_AFFINITY_ENABLED);
> - }
> - mem_base = 1ULL << 32;
> - mem_len = next_base - pcms->below_4g_mem_size;
> - next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> + }
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
> +
> + /* map the rest of memory from 1M */
> + mem_base = 1024 * 1024;
> + mem_len = pcms->node_mem[node] - mem_base;
> + mem_base = build_srat_node_entry(table_data, pcms, node,
> + mem_base, mem_len);
> +
> + for (i = 0; i < pcms->numa_nodes; i++) {
> + if (i == node) {
> + continue;
> }
> - numamem = acpi_data_push(table_data, sizeof *numamem);
> - build_srat_memory(numamem, mem_base, mem_len, i - 1,
> - MEM_AFFINITY_ENABLED);
> + mem_base = build_srat_node_entry(table_data, pcms, i,
> + mem_base, pcms->node_mem[i]);
> }
> slots = (table_data->len - numa_start) / sizeof *numamem;
> for (; slots < pcms->numa_nodes + 2; slots++) {
> --
> 2.5.5
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-18 18:35 ` Michael S. Tsirkin
@ 2017-08-18 19:20 ` Eduardo Habkost
0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2017-08-18 19:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Dou Liyang, qemu-devel, pbonzini, rth, imammedo
On Fri, Aug 18, 2017 at 09:35:00PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 16, 2017 at 09:45:35AM +0800, Dou Liyang wrote:
> > Currently, Using the fisrt node without memory on the machine makes
> > QEMU unhappy. With this example command line:
> > ... \
> > -m 1024M,slots=4,maxmem=32G \
> > -numa node,nodeid=0 \
> > -numa node,mem=1024M,nodeid=1 \
> > -numa node,nodeid=2 \
> > -numa node,nodeid=3 \
> > Guest reports "No NUMA configuration found" and the NUMA topology is
> > wrong.
> >
> > This is because when QEMU builds ACPI SRAT, it regards node0 as the
> > default node to deal with the memory hole(640K-1M). this means the
> > node0 must have some memory(>1M), but, actually it can have no
> > memory.
> >
> > Fix this problem by replace the node0 with the first node which has
> > memory on it. Add a new function for each node. Also do some cleanup.
> >
> > Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>
> This isn't a regression, is it? If it isn't, it's not a 2.10 candidate
> IMHO.
As noted in another reply to v2, I agree and I'm treating it as a
2.11 candidate.
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-18 16:48 ` Eduardo Habkost
@ 2017-08-20 11:52 ` Dou Liyang
2017-08-21 8:57 ` Igor Mammedov
0 siblings, 1 reply; 9+ messages in thread
From: Dou Liyang @ 2017-08-20 11:52 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, imammedo, pbonzini, rth, mst
Hi Eduardo,
At 08/19/2017 12:48 AM, Eduardo Habkost wrote:
> On Thu, Aug 17, 2017 at 01:58:40PM +0800, Dou Liyang wrote:
>> Hi Igor,
>>
>> I tested this patch with following guests:
>>
>> 1. RHEL 6.5 with Linux 2.6.32
>> 2. RHEL 7.0 with Linux 3.10.0
>> 3. Fedora 23 with Linux 4.13.0-rc5
>> 4. window 2003 service
>> 5. window 7
>> 6. window 10
>
> What's the command-line(s) you have tested with each OS? Have
>
I am sorry, one of the command-lines for the node0-without-RAM:
./x86_64-softmmu/qemu-system-x86_64 \
-hda /home/douly/image/rhel6.5.qcow2 \
-m 2G,slots=4,maxmem=32G \
-enable-kvm \
-smp 2,maxcpus=8,sockets=2,cores=2,threads=2\
-object memory-backend-ram,id=mem0,size=1024M \
-object memory-backend-ram,id=mem1,size=1024M \
-object memory-backend-ram,id=mem2,size=1024M \
-object memory-backend-ram,id=mem3,size=1024M \
-device pc-dimm,id=dimm0,memdev=mem0,slot=0,node=0 \
-device pc-dimm,id=dimm1,memdev=mem1,slot=1,node=1 \
-device pc-dimm,id=dimm2,memdev=mem2,slot=2,node=2 \
-device pc-dimm,id=dimm3,memdev=mem3,slot=3,node=3 \
-device qemu64-x86_64-cpu,id=cpu2,socket-id=0,core-id=1,thread-id=0 \
-device qemu64-x86_64-cpu,id=cpu3,socket-id=0,core-id=1,thread-id=1 \
-device qemu64-x86_64-cpu,id=cpu4,socket-id=1,core-id=0,thread-id=0 \
-device qemu64-x86_64-cpu,id=cpu5,socket-id=1,core-id=0,thread-id=1 \
-device qemu64-x86_64-cpu,id=cpu6,socket-id=1,core-id=1,thread-id=0 \
-device qemu64-x86_64-cpu,id=cpu7,socket-id=1,core-id=1,thread-id=1 \
-numa node,nodeid=0,cpus=0-1 \
-numa node,mem=2G,nodeid=1,cpus=2-3 \
-numa node,nodeid=2,cpus=4-5 \
-numa node,nodeid=3,cpus=6-7 \
-serial stdio \
-monitor telnet:127.0.0.1:4444,server,nowait \
# -kernel /home/douly/openSource/linux/arch/x86_64/boot/bzImage \
# -append "root=/dev/mapper/fedora_s3101490-root console=tty0
console=ttyS0 earlyprintk=ttyS0,115200n8 movable_node" \
# -initrd /home/douly/openSource/initramfs_image/4.13.0.img \
you tested both the node0-with-RAM and node0-without-RAM cases?
>
Yes, I tested three cases for each guests:
Case 1) all node with RAM.
-numa node,nodeid=0,cpus=0-1 \
-numa node,nodeid=1,cpus=2-3 \
-numa node,nodeid=2,cpus=4-5 \
-numa node,nodeid=3,cpus=6-7 \
Case 2) the node0-with-RAM:
-numa node,mem=2G,nodeid=0,cpus=0-1 \
-numa node,nodeid=1,cpus=2-3 \
-numa node,nodeid=2,cpus=4-5 \
-numa node,nodeid=3,cpus=6-7 \
Case 3) the node0-without-RAM:
-numa node,nodeid=0,cpus=0-1 \
-numa node,mem=2G,nodeid=1,cpus=2-3 \
-numa node,nodeid=2,cpus=4-5 \
-numa node,nodeid=3,cpus=6-7 \
> I would be interested in a demonstration that no bytes in the
> ACPI table are changed by this patch when there is some RAM
Yes, you are right, there is no change if node 0 has some RAM.
This patch just works for the node0-without-RAM situation.
> configured in node 0. (Is that already covered by our existing
> ACPI test cases?)
I am not sure it, could you tell me where are the SRAT test cases?
Seems there is no test cases about SRAT in bios-tables-test.c.
>
> A new test case in bios-tables-test.c for the bug you are fixing
> would be nice to have.
>
Ok, I will do it right now.
Thanks,
dou.
>>
>> Thanks,
>> dou.
>>
>> At 08/16/2017 09:45 AM, Dou Liyang wrote:
>>> Currently, Using the fisrt node without memory on the machine makes
>>> QEMU unhappy. With this example command line:
>>> ... \
>>> -m 1024M,slots=4,maxmem=32G \
>>> -numa node,nodeid=0 \
>>> -numa node,mem=1024M,nodeid=1 \
>>> -numa node,nodeid=2 \
>>> -numa node,nodeid=3 \
>>> Guest reports "No NUMA configuration found" and the NUMA topology is
>>> wrong.
>>>
>>> This is because when QEMU builds ACPI SRAT, it regards node0 as the
>>> default node to deal with the memory hole(640K-1M). this means the
>>> node0 must have some memory(>1M), but, actually it can have no
>>> memory.
>>>
>>> Fix this problem by replace the node0 with the first node which has
>>> memory on it. Add a new function for each node. Also do some cleanup.
>>>
>>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>>> ---
>>> V3 --> V2
>>> -Modify the title
>>> V2 --> V1:
>>> -Fix a coding style problem
>>> Replace
>>> for (node = 0;
>>> node < pcms->numa_nodes && pcms->node_mem[node] == 0;
>>> node++);
>>>
>>> with
>>> for (node = 0; node < pcms->numa_nodes; node++) {
>>> if (pcms->node_mem[node] != 0) {
>>> break;
>>> }
>>>
>>> hw/i386/acpi-build.c | 78 +++++++++++++++++++++++++++++++++-------------------
>>> 1 file changed, 50 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 98dd424..f93d712 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
>>> (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>>> }
>>>
>>> +static uint64_t
>>> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
>>> + int i, uint64_t mem_base, uint64_t mem_len)
>>> +{
>>> + AcpiSratMemoryAffinity *numamem;
>>> + uint64_t next_base;
>>> +
>>> + next_base = mem_base + mem_len;
>>> +
>>> + /* Cut out the ACPI_PCI hole */
>>> + if (mem_base <= pcms->below_4g_mem_size &&
>>> + next_base > pcms->below_4g_mem_size) {
>>> + 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,
>>> + MEM_AFFINITY_ENABLED);
>>> + }
>>> + mem_base = 1ULL << 32;
>>> + mem_len = next_base - pcms->below_4g_mem_size;
>>> + next_base += (1ULL << 32) - pcms->below_4g_mem_size;
>>> + }
>>> + numamem = acpi_data_push(table_data, sizeof *numamem);
>>> + build_srat_memory(numamem, mem_base, mem_len, i,
>>> + MEM_AFFINITY_ENABLED);
>>> + return next_base;
>>> +}
>>> +
>>> static void
>>> build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>> {
>>> AcpiSystemResourceAffinityTable *srat;
>>> AcpiSratMemoryAffinity *numamem;
>>>
>>> - int i;
>>> + int i, node;
>>> int srat_start, numa_start, slots;
>>> - uint64_t mem_len, mem_base, next_base;
>>> + uint64_t mem_len, mem_base;
>>> MachineClass *mc = MACHINE_GET_CLASS(machine);
>>> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
>>> PCMachineState *pcms = PC_MACHINE(machine);
>>> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>> /* the memory map is a bit tricky, it contains at least one hole
>>> * from 640k-1M and possibly another one from 3.5G-4G.
>>> */
>>> - next_base = 0;
>>> +
>>> numa_start = table_data->len;
>>>
>>> - numamem = acpi_data_push(table_data, sizeof *numamem);
>>> - build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
>>> - next_base = 1024 * 1024;
>>> - for (i = 1; i < pcms->numa_nodes + 1; ++i) {
>>> - mem_base = next_base;
>>> - mem_len = pcms->node_mem[i - 1];
>>> - if (i == 1) {
>>> - mem_len -= 1024 * 1024;
>>> + /* get the first node which has memory and map the hole from 640K-1M */
>>> + for (node = 0; node < pcms->numa_nodes; node++) {
>>> + if (pcms->node_mem[node] != 0) {
>>> + break;
>>> }
>>> - next_base = mem_base + mem_len;
>>> -
>>> - /* Cut out the ACPI_PCI hole */
>>> - if (mem_base <= pcms->below_4g_mem_size &&
>>> - next_base > pcms->below_4g_mem_size) {
>>> - 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,
>>> - MEM_AFFINITY_ENABLED);
>>> - }
>>> - mem_base = 1ULL << 32;
>>> - mem_len = next_base - pcms->below_4g_mem_size;
>>> - next_base += (1ULL << 32) - pcms->below_4g_mem_size;
>>> + }
>>> + numamem = acpi_data_push(table_data, sizeof *numamem);
>>> + build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
>>> +
>>> + /* map the rest of memory from 1M */
>>> + mem_base = 1024 * 1024;
>>> + mem_len = pcms->node_mem[node] - mem_base;
>>> + mem_base = build_srat_node_entry(table_data, pcms, node,
>>> + mem_base, mem_len);
>>> +
>>> + for (i = 0; i < pcms->numa_nodes; i++) {
>>> + if (i == node) {
>>> + continue;
>>> }
>>> - numamem = acpi_data_push(table_data, sizeof *numamem);
>>> - build_srat_memory(numamem, mem_base, mem_len, i - 1,
>>> - MEM_AFFINITY_ENABLED);
>>> + mem_base = build_srat_node_entry(table_data, pcms, i,
>>> + mem_base, pcms->node_mem[i]);
>>> }
>>> slots = (table_data->len - numa_start) / sizeof *numamem;
>>> for (; slots < pcms->numa_nodes + 2; slots++) {
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-20 11:52 ` Dou Liyang
@ 2017-08-21 8:57 ` Igor Mammedov
2017-08-21 11:23 ` Dou Liyang
0 siblings, 1 reply; 9+ messages in thread
From: Igor Mammedov @ 2017-08-21 8:57 UTC (permalink / raw)
To: Dou Liyang; +Cc: Eduardo Habkost, qemu-devel, pbonzini, rth, mst
On Sun, 20 Aug 2017 19:52:11 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
> Hi Eduardo,
>
> At 08/19/2017 12:48 AM, Eduardo Habkost wrote:
> > On Thu, Aug 17, 2017 at 01:58:40PM +0800, Dou Liyang wrote:
> >> Hi Igor,
> >>
> >> I tested this patch with following guests:
> >>
> >> 1. RHEL 6.5 with Linux 2.6.32
> >> 2. RHEL 7.0 with Linux 3.10.0
> >> 3. Fedora 23 with Linux 4.13.0-rc5
> >> 4. window 2003 service
> >> 5. window 7
> >> 6. window 10
> >
> > What's the command-line(s) you have tested with each OS? Have
> >
>
> I am sorry, one of the command-lines for the node0-without-RAM:
>
> ./x86_64-softmmu/qemu-system-x86_64 \
> -hda /home/douly/image/rhel6.5.qcow2 \
> -m 2G,slots=4,maxmem=32G \
> -enable-kvm \
> -smp 2,maxcpus=8,sockets=2,cores=2,threads=2\
> -object memory-backend-ram,id=mem0,size=1024M \
> -object memory-backend-ram,id=mem1,size=1024M \
> -object memory-backend-ram,id=mem2,size=1024M \
> -object memory-backend-ram,id=mem3,size=1024M \
> -device pc-dimm,id=dimm0,memdev=mem0,slot=0,node=0 \
> -device pc-dimm,id=dimm1,memdev=mem1,slot=1,node=1 \
> -device pc-dimm,id=dimm2,memdev=mem2,slot=2,node=2 \
> -device pc-dimm,id=dimm3,memdev=mem3,slot=3,node=3 \
> -device qemu64-x86_64-cpu,id=cpu2,socket-id=0,core-id=1,thread-id=0 \
> -device qemu64-x86_64-cpu,id=cpu3,socket-id=0,core-id=1,thread-id=1 \
> -device qemu64-x86_64-cpu,id=cpu4,socket-id=1,core-id=0,thread-id=0 \
> -device qemu64-x86_64-cpu,id=cpu5,socket-id=1,core-id=0,thread-id=1 \
> -device qemu64-x86_64-cpu,id=cpu6,socket-id=1,core-id=1,thread-id=0 \
> -device qemu64-x86_64-cpu,id=cpu7,socket-id=1,core-id=1,thread-id=1 \
> -numa node,nodeid=0,cpus=0-1 \
> -numa node,mem=2G,nodeid=1,cpus=2-3 \
> -numa node,nodeid=2,cpus=4-5 \
> -numa node,nodeid=3,cpus=6-7 \
> -serial stdio \
> -monitor telnet:127.0.0.1:4444,server,nowait \
> # -kernel /home/douly/openSource/linux/arch/x86_64/boot/bzImage \
> # -append "root=/dev/mapper/fedora_s3101490-root console=tty0
> console=ttyS0 earlyprintk=ttyS0,115200n8 movable_node" \
> # -initrd /home/douly/openSource/initramfs_image/4.13.0.img \
> you tested both the node0-with-RAM and node0-without-RAM cases?
> >
>
> Yes, I tested three cases for each guests:
>
> Case 1) all node with RAM.
>
> -numa node,nodeid=0,cpus=0-1 \
> -numa node,nodeid=1,cpus=2-3 \
> -numa node,nodeid=2,cpus=4-5 \
> -numa node,nodeid=3,cpus=6-7 \
>
> Case 2) the node0-with-RAM:
>
> -numa node,mem=2G,nodeid=0,cpus=0-1 \
> -numa node,nodeid=1,cpus=2-3 \
> -numa node,nodeid=2,cpus=4-5 \
> -numa node,nodeid=3,cpus=6-7 \
>
> Case 3) the node0-without-RAM:
>
> -numa node,nodeid=0,cpus=0-1 \
> -numa node,mem=2G,nodeid=1,cpus=2-3 \
> -numa node,nodeid=2,cpus=4-5 \
> -numa node,nodeid=3,cpus=6-7 \
>
> > I would be interested in a demonstration that no bytes in the
> > ACPI table are changed by this patch when there is some RAM
>
> Yes, you are right, there is no change if node 0 has some RAM.
> This patch just works for the node0-without-RAM situation.
>
> > configured in node 0. (Is that already covered by our existing
> > ACPI test cases?)
>
> I am not sure it, could you tell me where are the SRAT test cases?
> Seems there is no test cases about SRAT in bios-tables-test.c.
>
> >
> > A new test case in bios-tables-test.c for the bug you are fixing
> > would be nice to have.
> >
>
> Ok, I will do it right now.
see commit 6b9c1dd2c for example of adding test case,
currently test has base SRAT tables that is reused by all tests.
When you create testcase that produces different SRAT table,
'make check' will print warnings that ables do not match
you can add "V=1" env. var. 'make V=1 check' to get detailed
diff of what's changed.
also see how 'test_data.variant' is handled to get idea how to use
it to differentiate specific test case vs shared ACPI tables baseline blobs.
>
> Thanks,
> dou.
> >>
>
>
> >> Thanks,
> >> dou.
> >>
> >> At 08/16/2017 09:45 AM, Dou Liyang wrote:
> >>> Currently, Using the fisrt node without memory on the machine makes
> >>> QEMU unhappy. With this example command line:
> >>> ... \
> >>> -m 1024M,slots=4,maxmem=32G \
> >>> -numa node,nodeid=0 \
> >>> -numa node,mem=1024M,nodeid=1 \
> >>> -numa node,nodeid=2 \
> >>> -numa node,nodeid=3 \
> >>> Guest reports "No NUMA configuration found" and the NUMA topology is
> >>> wrong.
> >>>
> >>> This is because when QEMU builds ACPI SRAT, it regards node0 as the
> >>> default node to deal with the memory hole(640K-1M). this means the
> >>> node0 must have some memory(>1M), but, actually it can have no
> >>> memory.
> >>>
> >>> Fix this problem by replace the node0 with the first node which has
> >>> memory on it. Add a new function for each node. Also do some cleanup.
> >>>
> >>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> >>> ---
> >>> V3 --> V2
> >>> -Modify the title
> >>> V2 --> V1:
> >>> -Fix a coding style problem
> >>> Replace
> >>> for (node = 0;
> >>> node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> >>> node++);
> >>>
> >>> with
> >>> for (node = 0; node < pcms->numa_nodes; node++) {
> >>> if (pcms->node_mem[node] != 0) {
> >>> break;
> >>> }
> >>>
> >>> hw/i386/acpi-build.c | 78 +++++++++++++++++++++++++++++++++-------------------
> >>> 1 file changed, 50 insertions(+), 28 deletions(-)
> >>>
> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>> index 98dd424..f93d712 100644
> >>> --- a/hw/i386/acpi-build.c
> >>> +++ b/hw/i386/acpi-build.c
> >>> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> >>> (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> >>> }
> >>>
> >>> +static uint64_t
> >>> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> >>> + int i, uint64_t mem_base, uint64_t mem_len)
> >>> +{
> >>> + AcpiSratMemoryAffinity *numamem;
> >>> + uint64_t next_base;
> >>> +
> >>> + next_base = mem_base + mem_len;
> >>> +
> >>> + /* Cut out the ACPI_PCI hole */
> >>> + if (mem_base <= pcms->below_4g_mem_size &&
> >>> + next_base > pcms->below_4g_mem_size) {
> >>> + 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,
> >>> + MEM_AFFINITY_ENABLED);
> >>> + }
> >>> + mem_base = 1ULL << 32;
> >>> + mem_len = next_base - pcms->below_4g_mem_size;
> >>> + next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> >>> + }
> >>> + numamem = acpi_data_push(table_data, sizeof *numamem);
> >>> + build_srat_memory(numamem, mem_base, mem_len, i,
> >>> + MEM_AFFINITY_ENABLED);
> >>> + return next_base;
> >>> +}
> >>> +
> >>> static void
> >>> build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> >>> {
> >>> AcpiSystemResourceAffinityTable *srat;
> >>> AcpiSratMemoryAffinity *numamem;
> >>>
> >>> - int i;
> >>> + int i, node;
> >>> int srat_start, numa_start, slots;
> >>> - uint64_t mem_len, mem_base, next_base;
> >>> + uint64_t mem_len, mem_base;
> >>> MachineClass *mc = MACHINE_GET_CLASS(machine);
> >>> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> >>> PCMachineState *pcms = PC_MACHINE(machine);
> >>> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> >>> /* the memory map is a bit tricky, it contains at least one hole
> >>> * from 640k-1M and possibly another one from 3.5G-4G.
> >>> */
> >>> - next_base = 0;
> >>> +
> >>> numa_start = table_data->len;
> >>>
> >>> - numamem = acpi_data_push(table_data, sizeof *numamem);
> >>> - build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> >>> - next_base = 1024 * 1024;
> >>> - for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> >>> - mem_base = next_base;
> >>> - mem_len = pcms->node_mem[i - 1];
> >>> - if (i == 1) {
> >>> - mem_len -= 1024 * 1024;
> >>> + /* get the first node which has memory and map the hole from 640K-1M */
> >>> + for (node = 0; node < pcms->numa_nodes; node++) {
> >>> + if (pcms->node_mem[node] != 0) {
> >>> + break;
> >>> }
> >>> - next_base = mem_base + mem_len;
> >>> -
> >>> - /* Cut out the ACPI_PCI hole */
> >>> - if (mem_base <= pcms->below_4g_mem_size &&
> >>> - next_base > pcms->below_4g_mem_size) {
> >>> - 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,
> >>> - MEM_AFFINITY_ENABLED);
> >>> - }
> >>> - mem_base = 1ULL << 32;
> >>> - mem_len = next_base - pcms->below_4g_mem_size;
> >>> - next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> >>> + }
> >>> + numamem = acpi_data_push(table_data, sizeof *numamem);
> >>> + build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
> >>> +
> >>> + /* map the rest of memory from 1M */
> >>> + mem_base = 1024 * 1024;
> >>> + mem_len = pcms->node_mem[node] - mem_base;
> >>> + mem_base = build_srat_node_entry(table_data, pcms, node,
> >>> + mem_base, mem_len);
> >>> +
> >>> + for (i = 0; i < pcms->numa_nodes; i++) {
> >>> + if (i == node) {
> >>> + continue;
> >>> }
> >>> - numamem = acpi_data_push(table_data, sizeof *numamem);
> >>> - build_srat_memory(numamem, mem_base, mem_len, i - 1,
> >>> - MEM_AFFINITY_ENABLED);
> >>> + mem_base = build_srat_node_entry(table_data, pcms, i,
> >>> + mem_base, pcms->node_mem[i]);
> >>> }
> >>> slots = (table_data->len - numa_start) / sizeof *numamem;
> >>> for (; slots < pcms->numa_nodes + 2; slots++) {
> >>>
> >>
> >>
> >
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-21 8:57 ` Igor Mammedov
@ 2017-08-21 11:23 ` Dou Liyang
2017-08-21 13:25 ` Igor Mammedov
0 siblings, 1 reply; 9+ messages in thread
From: Dou Liyang @ 2017-08-21 11:23 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Eduardo Habkost, qemu-devel, pbonzini, rth, mst
Hi, Igor
[...]
>>
>> Ok, I will do it right now.
>
> see commit 6b9c1dd2c for example of adding test case,
> currently test has base SRAT tables that is reused by all tests.
> When you create testcase that produces different SRAT table,
> 'make check' will print warnings that ables do not match
> you can add "V=1" env. var. 'make V=1 check' to get detailed
> diff of what's changed.
>
> also see how 'test_data.variant' is handled to get idea how to use
> it to differentiate specific test case vs shared ACPI tables baseline blobs.
>
Thanks for teaching me that, it's very helpful to me.
Except for adding a new case, can we except memhp testcase with
node RAM check? just like following shows:
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 564da45..c79ece4 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -788,7 +788,7 @@ static void test_acpi_q35_tcg_memhp(void)
data.machine = MACHINE_Q35;
data.variant = ".memhp";
test_acpi_one(" -m 128,slots=3,maxmem=1G"
- " -numa node -numa node"
+ " -numa node -numa node,mem=128"
" -numa dist,src=0,dst=1,val=21",
&data);
free_test_data(&data);
@@ -802,7 +802,7 @@ static void test_acpi_piix4_tcg_memhp(void)
data.machine = MACHINE_PC;
data.variant = ".memhp";
test_acpi_one(" -m 128,slots=3,maxmem=1G"
- " -numa node -numa node"
+ " -numa node -numa node,mem=128"
" -numa dist,src=0,dst=1,val=21",
&data);
free_test_data(&data);
Thanks,
dou.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
2017-08-21 11:23 ` Dou Liyang
@ 2017-08-21 13:25 ` Igor Mammedov
0 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2017-08-21 13:25 UTC (permalink / raw)
To: Dou Liyang; +Cc: Eduardo Habkost, qemu-devel, pbonzini, rth, mst
On Mon, 21 Aug 2017 19:23:35 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
> Hi, Igor
>
> [...]
> >>
> >> Ok, I will do it right now.
> >
> > see commit 6b9c1dd2c for example of adding test case,
> > currently test has base SRAT tables that is reused by all tests.
> > When you create testcase that produces different SRAT table,
> > 'make check' will print warnings that ables do not match
> > you can add "V=1" env. var. 'make V=1 check' to get detailed
> > diff of what's changed.
> >
> > also see how 'test_data.variant' is handled to get idea how to use
> > it to differentiate specific test case vs shared ACPI tables baseline blobs.
> >
>
> Thanks for teaching me that, it's very helpful to me.
>
> Except for adding a new case, can we except memhp testcase with
> node RAM check? just like following shows:
then you'll lose SRAT with even distribution between nodes.
Just add an additional variant for your usecase.
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 564da45..c79ece4 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -788,7 +788,7 @@ static void test_acpi_q35_tcg_memhp(void)
> data.machine = MACHINE_Q35;
> data.variant = ".memhp";
> test_acpi_one(" -m 128,slots=3,maxmem=1G"
> - " -numa node -numa node"
> + " -numa node -numa node,mem=128"
> " -numa dist,src=0,dst=1,val=21",
> &data);
> free_test_data(&data);
> @@ -802,7 +802,7 @@ static void test_acpi_piix4_tcg_memhp(void)
> data.machine = MACHINE_PC;
> data.variant = ".memhp";
> test_acpi_one(" -m 128,slots=3,maxmem=1G"
> - " -numa node -numa node"
> + " -numa node -numa node,mem=128"
> " -numa dist,src=0,dst=1,val=21",
> &data);
> free_test_data(&data);
>
>
> Thanks,
> dou.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-21 13:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-16 1:45 [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0 Dou Liyang
2017-08-17 5:58 ` Dou Liyang
2017-08-18 16:48 ` Eduardo Habkost
2017-08-20 11:52 ` Dou Liyang
2017-08-21 8:57 ` Igor Mammedov
2017-08-21 11:23 ` Dou Liyang
2017-08-21 13:25 ` Igor Mammedov
2017-08-18 18:35 ` Michael S. Tsirkin
2017-08-18 19:20 ` Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.