diff mbox

[v5,1/3] hw/acpi-build: Fix SRAT memory building in case of node 0 without RAM

Message ID 1504181068-17822-2-git-send-email-douly.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dou Liyang Aug. 31, 2017, 12:04 p.m. UTC
From: Eduardo Habkost <ehabkost@redhat.com>

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 node 0 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  cut out the 640K hole in the same way the PCI
4G hole does. Also do some cleanup.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

Comments

Eduardo Habkost Aug. 31, 2017, 9:36 p.m. UTC | #1
On Thu, Aug 31, 2017 at 08:04:26PM +0800, Dou Liyang wrote:
> From: Eduardo Habkost <ehabkost@redhat.com>
> 
> 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 node 0 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  cut out the 640K hole in the same way the PCI
> 4G hole does. Also do some cleanup.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..48525a1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,6 +2318,9 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
>                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>  }
>  
> +#define HOLE_640K_START  (640 * 1024)
> +#define HOLE_640K_END   (1024 * 1024)
> +
>  static void
>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>  {
> @@ -2373,17 +2376,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>      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;
> -        }
>          next_base = mem_base + mem_len;
>  
> +        /* Cut out the 640K hole */
> +        if (mem_base <= HOLE_640K_START &&
> +            next_base > HOLE_640K_START) {
> +            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,
> +                                  MEM_AFFINITY_ENABLED);
> +            }
> +
> +            /* Check for the rare case: 640K < RAM < 1M */
> +            if (next_base <= HOLE_640K_END) {
> +                next_base = HOLE_640K_END;
> +                continue;
> +            }
> +            mem_base = HOLE_640K_END;
> +            mem_len = next_base - HOLE_640K_END;
> +        }
> +
>          /* Cut out the ACPI_PCI hole */
>          if (mem_base <= pcms->below_4g_mem_size &&
>              next_base > pcms->below_4g_mem_size) {
> @@ -2395,7 +2411,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>              }
>              mem_base = 1ULL << 32;
>              mem_len = next_base - pcms->below_4g_mem_size;
> -            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +            next_base = mem_base + mem_len;

Is this extra change intentional?

I find the code more readable with it, but it should go in a
separate patch because it is unrelated to the bug fix.
Dou Liyang Sept. 1, 2017, 1:29 a.m. UTC | #2
Hi, Eduardo

At 09/01/2017 05:36 AM, Eduardo Habkost wrote:
> On Thu, Aug 31, 2017 at 08:04:26PM +0800, Dou Liyang wrote:
>> From: Eduardo Habkost <ehabkost@redhat.com>
>>
>> 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 node 0 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  cut out the 640K hole in the same way the PCI
>> 4G hole does. Also do some cleanup.
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>> ---
>>  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
>>  1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 98dd424..48525a1 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2318,6 +2318,9 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
>>                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>>  }
>>
>> +#define HOLE_640K_START  (640 * 1024)
>> +#define HOLE_640K_END   (1024 * 1024)
>> +
>>  static void
>>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>  {
>> @@ -2373,17 +2376,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>      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;
>> -        }
>>          next_base = mem_base + mem_len;
>>
>> +        /* Cut out the 640K hole */
>> +        if (mem_base <= HOLE_640K_START &&
>> +            next_base > HOLE_640K_START) {
>> +            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,
>> +                                  MEM_AFFINITY_ENABLED);
>> +            }
>> +
>> +            /* Check for the rare case: 640K < RAM < 1M */
>> +            if (next_base <= HOLE_640K_END) {
>> +                next_base = HOLE_640K_END;
>> +                continue;
>> +            }
>> +            mem_base = HOLE_640K_END;
>> +            mem_len = next_base - HOLE_640K_END;
>> +        }
>> +
>>          /* Cut out the ACPI_PCI hole */
>>          if (mem_base <= pcms->below_4g_mem_size &&
>>              next_base > pcms->below_4g_mem_size) {
>> @@ -2395,7 +2411,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>              }
>>              mem_base = 1ULL << 32;
>>              mem_len = next_base - pcms->below_4g_mem_size;
>> -            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
>> +            next_base = mem_base + mem_len;
>
> Is this extra change intentional?
>

Yes, it is, Just for readability. :-)

> I find the code more readable with it, but it should go in a
> separate patch because it is unrelated to the bug fix.
>

Indeed, I will split it out.

Thanks,
	dou.
Igor Mammedov Sept. 4, 2017, 9:39 a.m. UTC | #3
On Thu, 31 Aug 2017 20:04:26 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> From: Eduardo Habkost <ehabkost@redhat.com>
> 
> 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 node 0 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  cut out the 640K hole in the same way the PCI
> 4G hole does. Also do some cleanup.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..48525a1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,6 +2318,9 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
>                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>  }
>  
> +#define HOLE_640K_START  (640 * 1024)
> +#define HOLE_640K_END   (1024 * 1024)
> +
>  static void
>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>  {
> @@ -2373,17 +2376,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>      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;
> -        }
>          next_base = mem_base + mem_len;
>  
> +        /* Cut out the 640K hole */
> +        if (mem_base <= HOLE_640K_START &&
> +            next_base > HOLE_640K_START) {
> +            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,
> +                                  MEM_AFFINITY_ENABLED);
> +            }
> +
> +            /* Check for the rare case: 640K < RAM < 1M */
> +            if (next_base <= HOLE_640K_END) {
> +                next_base = HOLE_640K_END;
Is this assignment really necessary?

it seems that next_base will be set at the start of the loop.

> +                continue;
> +            }
> +            mem_base = HOLE_640K_END;
> +            mem_len = next_base - HOLE_640K_END;
> +        }
> +
>          /* Cut out the ACPI_PCI hole */
>          if (mem_base <= pcms->below_4g_mem_size &&
>              next_base > pcms->below_4g_mem_size) {
> @@ -2395,7 +2411,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>              }
>              mem_base = 1ULL << 32;
>              mem_len = next_base - pcms->below_4g_mem_size;
> -            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +            next_base = mem_base + mem_len;
>          }
>          numamem = acpi_data_push(table_data, sizeof *numamem);
>          build_srat_memory(numamem, mem_base, mem_len, i - 1,
Dou Liyang Sept. 4, 2017, 10:16 a.m. UTC | #4
At 09/04/2017 05:39 PM, Igor Mammedov wrote:
> On Thu, 31 Aug 2017 20:04:26 +0800
> Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
>
>> From: Eduardo Habkost <ehabkost@redhat.com>
>>
>> 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 node 0 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  cut out the 640K hole in the same way the PCI
>> 4G hole does. Also do some cleanup.
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>> ---
>>  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
>>  1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 98dd424..48525a1 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2318,6 +2318,9 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
>>                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>>  }
>>
>> +#define HOLE_640K_START  (640 * 1024)
>> +#define HOLE_640K_END   (1024 * 1024)
>> +
>>  static void
>>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>  {
>> @@ -2373,17 +2376,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>      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;
>> -        }
>>          next_base = mem_base + mem_len;
>>
>> +        /* Cut out the 640K hole */
>> +        if (mem_base <= HOLE_640K_START &&
>> +            next_base > HOLE_640K_START) {
>> +            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,
>> +                                  MEM_AFFINITY_ENABLED);
>> +            }
>> +
>> +            /* Check for the rare case: 640K < RAM < 1M */
>> +            if (next_base <= HOLE_640K_END) {
>> +                next_base = HOLE_640K_END;
> Is this assignment really necessary?
>

It is necessary, because we set mem_base to next_base before setting
next_base;

But, I can refine it:

                                    MEM_AFFINITY_ENABLED);
              }

+            mem_base = HOLE_640K_END;
              /* Check for the rare case: 640K < RAM < 1M */
              if (next_base <= HOLE_640K_END) {
-                next_base = HOLE_640K_END;
                  continue;
              }
-            mem_base = HOLE_640K_END;
              mem_len = next_base - HOLE_640K_END;
          }

Is it?

Thanks,
	dou.

> it seems that next_base will be set at the start of the loop.
>


>> +                continue;
>> +            }
>> +            mem_base = HOLE_640K_END;
>> +            mem_len = next_base - HOLE_640K_END;
>> +        }
>> +
>>          /* Cut out the ACPI_PCI hole */
>>          if (mem_base <= pcms->below_4g_mem_size &&
>>              next_base > pcms->below_4g_mem_size) {
>> @@ -2395,7 +2411,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>>              }
>>              mem_base = 1ULL << 32;
>>              mem_len = next_base - pcms->below_4g_mem_size;
>> -            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
>> +            next_base = mem_base + mem_len;
>>          }
>>          numamem = acpi_data_push(table_data, sizeof *numamem);
>>          build_srat_memory(numamem, mem_base, mem_len, i - 1,
>
>
>
>
Igor Mammedov Sept. 4, 2017, 11:11 a.m. UTC | #5
On Mon, 4 Sep 2017 18:16:31 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> At 09/04/2017 05:39 PM, Igor Mammedov wrote:
> > On Thu, 31 Aug 2017 20:04:26 +0800
> > Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
> >  
> >> From: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >> 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 node 0 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  cut out the 640K hole in the same way the PCI
> >> 4G hole does. Also do some cleanup.
> >>
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> >> ---
> >>  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
> >>  1 file changed, 23 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 98dd424..48525a1 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2318,6 +2318,9 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> >>                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> >>  }
> >>
> >> +#define HOLE_640K_START  (640 * 1024)
> >> +#define HOLE_640K_END   (1024 * 1024)
> >> +
> >>  static void
> >>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> >>  {
> >> @@ -2373,17 +2376,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> >>      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;
> >> -        }
> >>          next_base = mem_base + mem_len;
> >>
> >> +        /* Cut out the 640K hole */
> >> +        if (mem_base <= HOLE_640K_START &&
> >> +            next_base > HOLE_640K_START) {
> >> +            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,
> >> +                                  MEM_AFFINITY_ENABLED);
> >> +            }
> >> +
> >> +            /* Check for the rare case: 640K < RAM < 1M */
> >> +            if (next_base <= HOLE_640K_END) {
> >> +                next_base = HOLE_640K_END;  
> > Is this assignment really necessary?
> >  
> 
> It is necessary, because we set mem_base to next_base before setting
> next_base;
> 
> But, I can refine it:
> 
>                                     MEM_AFFINITY_ENABLED);
>               }
> 
> +            mem_base = HOLE_640K_END;
>               /* Check for the rare case: 640K < RAM < 1M */
>               if (next_base <= HOLE_640K_END) {
> -                next_base = HOLE_640K_END;
>                   continue;
>               }
> -            mem_base = HOLE_640K_END;
>               mem_len = next_base - HOLE_640K_END;
>           }
> 
> Is it?
I was wrong, so just leave it as it is now.

> 
> Thanks,
> 	dou.
> 
> > it seems that next_base will be set at the start of the loop.
> >  
> 
> 
> >> +                continue;
> >> +            }
> >> +            mem_base = HOLE_640K_END;
> >> +            mem_len = next_base - HOLE_640K_END;
> >> +        }
> >> +
> >>          /* Cut out the ACPI_PCI hole */
> >>          if (mem_base <= pcms->below_4g_mem_size &&
> >>              next_base > pcms->below_4g_mem_size) {
> >> @@ -2395,7 +2411,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> >>              }
> >>              mem_base = 1ULL << 32;
> >>              mem_len = next_base - pcms->below_4g_mem_size;
> >> -            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> >> +            next_base = mem_base + mem_len;
> >>          }
> >>          numamem = acpi_data_push(table_data, sizeof *numamem);
> >>          build_srat_memory(numamem, mem_base, mem_len, i - 1,  
> >
> >
> >
> >  
> 
>
Dou Liyang Sept. 5, 2017, 12:59 a.m. UTC | #6
Hi Igor,

At 09/04/2017 07:11 PM, Igor Mammedov wrote:
[...]
>>>> +        if (mem_base <= HOLE_640K_START &&
>>>> +            next_base > HOLE_640K_START) {
>>>> +            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,
>>>> +                                  MEM_AFFINITY_ENABLED);
>>>> +            }
>>>> +
>>>> +            /* Check for the rare case: 640K < RAM < 1M */
>>>> +            if (next_base <= HOLE_640K_END) {
>>>> +                next_base = HOLE_640K_END;
>>> Is this assignment really necessary?
>>>
>>
>> It is necessary, because we set mem_base to next_base before setting
>> next_base;
>>
>> But, I can refine it:
>>
>>                                     MEM_AFFINITY_ENABLED);
>>               }
>>
>> +            mem_base = HOLE_640K_END;
>>               /* Check for the rare case: 640K < RAM < 1M */
>>               if (next_base <= HOLE_640K_END) {
>> -                next_base = HOLE_640K_END;
>>                   continue;
>>               }
>> -            mem_base = HOLE_640K_END;
>>               mem_len = next_base - HOLE_640K_END;
>>           }
>>
>> Is it?
> I was wrong, so just leave it as it is now.
>

OK, I see.

Thanks,
	dou.
diff mbox

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 98dd424..48525a1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2318,6 +2318,9 @@  build_tpm2(GArray *table_data, BIOSLinker *linker)
                  (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
 }
 
+#define HOLE_640K_START  (640 * 1024)
+#define HOLE_640K_END   (1024 * 1024)
+
 static void
 build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 {
@@ -2373,17 +2376,30 @@  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
     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;
-        }
         next_base = mem_base + mem_len;
 
+        /* Cut out the 640K hole */
+        if (mem_base <= HOLE_640K_START &&
+            next_base > HOLE_640K_START) {
+            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,
+                                  MEM_AFFINITY_ENABLED);
+            }
+
+            /* Check for the rare case: 640K < RAM < 1M */
+            if (next_base <= HOLE_640K_END) {
+                next_base = HOLE_640K_END;
+                continue;
+            }
+            mem_base = HOLE_640K_END;
+            mem_len = next_base - HOLE_640K_END;
+        }
+
         /* Cut out the ACPI_PCI hole */
         if (mem_base <= pcms->below_4g_mem_size &&
             next_base > pcms->below_4g_mem_size) {
@@ -2395,7 +2411,7 @@  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
             }
             mem_base = 1ULL << 32;
             mem_len = next_base - pcms->below_4g_mem_size;
-            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+            next_base = mem_base + mem_len;
         }
         numamem = acpi_data_push(table_data, sizeof *numamem);
         build_srat_memory(numamem, mem_base, mem_len, i - 1,