In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT table
for transfering NUMA configuration to the guest. So, the maximum memory in
SRAT can be used to determine whether to use the swiotlb for IOMMU or not.

However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
never be built. When memory hotplug is enabled, some guest's devices may
start failing due to SWIOTLB is disabled.

Add numa_implicit_add_node0 in struct MachineClass, Invoke it before QEMU
parse NUMA options to enable adding NUMA node implicitly.

Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Alistair Francis <alistair23@gmail.com>
Cc: f4bug@amsat.org
Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>

---
 hw/i386/pc.c        |  6 ++++++
 include/hw/boards.h |  4 ++++
 vl.c                | 14 ++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2108104..3c40117 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2308,6 +2308,11 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
     return ms->possible_cpus;
 }
 
+static void numa_implicit_add_node0(void)
+{
+    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
+}
+
 static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
 {
     /* cpu index isn't used */
@@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
     mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
+    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
     mc->has_hotpluggable_cpus = true;
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 7f044d1..898d841 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -141,6 +141,8 @@ typedef struct {
  *    should instead use "unimplemented-device" for all memory ranges where
  *    the guest will attempt to probe for a device that QEMU doesn't
  *    implement and a stub device is required.
+ * @numa_implicit_add_node0:
+ *    Enable NUMA implicitly by add a NUMA node.
  */
 struct MachineClass {
     /*< private >*/
@@ -191,6 +193,8 @@ struct MachineClass {
     CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
                                                          unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
+
+    void (*numa_implicit_add_node0)(void);
 };
 
 /**
diff --git a/vl.c b/vl.c
index fb1f05b..814a5fa 100644
--- a/vl.c
+++ b/vl.c
@@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
     Error *main_loop_err = NULL;
     Error *err = NULL;
     bool list_data_dirs = false;
+    bool has_numa_config_in_CLI = false;
     typedef struct BlockdevOptions_queue {
         BlockdevOptions *bdo;
         Location loc;
@@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
                 if (!opts) {
                     exit(1);
                 }
+                has_numa_config_in_CLI = true;
                 break;
             case QEMU_OPTION_display:
                 display_type = select_display(optarg);
@@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
+    /*
+     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
+     * NUMA nodes explicitly on CLI
+     *
+     * Enable NUMA implicitly for guest to know the maximum memory
+     * from ACPI SRAT table, which is used for SWIOTLB.
+     */
+    if (ram_slots > 0 && !has_numa_config_in_CLI) {
+        if (machine_class->numa_implicit_add_node0) {
+            machine_class->numa_implicit_add_node0();
+        }
+    }
     parse_numa_opts(current_machine);
 
     if (qemu_opts_foreach(qemu_find_opts("mon"),
-- 
2.5.5

On Fri, Sep 15, 2017 at 04:33:18PM +0800, Dou Liyang wrote:
> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT table
> for transfering NUMA configuration to the guest. So, the maximum memory in
> SRAT can be used to determine whether to use the swiotlb for IOMMU or not.
> 
> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
> never be built. When memory hotplug is enabled, some guest's devices may
> start failing due to SWIOTLB is disabled.
> 
> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before QEMU
> parse NUMA options to enable adding NUMA node implicitly.
> 
> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Alistair Francis <alistair23@gmail.com>
> Cc: f4bug@amsat.org
> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
> 
> ---
>  hw/i386/pc.c        |  6 ++++++
>  include/hw/boards.h |  4 ++++
>  vl.c                | 14 ++++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2108104..3c40117 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2308,6 +2308,11 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
>      return ms->possible_cpus;
>  }
>  
> +static void numa_implicit_add_node0(void)
> +{
> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
> +}
> +
>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>  {
>      /* cpu index isn't used */
> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
>      mc->has_hotpluggable_cpus = true;
>      mc->default_boot_order = "cad";
>      mc->hot_add_cpu = pc_hot_add_cpu;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7f044d1..898d841 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -141,6 +141,8 @@ typedef struct {
>   *    should instead use "unimplemented-device" for all memory ranges where
>   *    the guest will attempt to probe for a device that QEMU doesn't
>   *    implement and a stub device is required.
> + * @numa_implicit_add_node0:
> + *    Enable NUMA implicitly by add a NUMA node.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -191,6 +193,8 @@ struct MachineClass {
>      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
>                                                           unsigned cpu_index);
>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> +
> +    void (*numa_implicit_add_node0)(void);
>  };
>  
>  /**
> diff --git a/vl.c b/vl.c
> index fb1f05b..814a5fa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>      Error *main_loop_err = NULL;
>      Error *err = NULL;
>      bool list_data_dirs = false;
> +    bool has_numa_config_in_CLI = false;
>      typedef struct BlockdevOptions_queue {
>          BlockdevOptions *bdo;
>          Location loc;
> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>                  if (!opts) {
>                      exit(1);
>                  }
> +                has_numa_config_in_CLI = true;
>                  break;
>              case QEMU_OPTION_display:
>                  display_type = select_display(optarg);
> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>  
> +    /*
> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
> +     * NUMA nodes explicitly on CLI
> +     *
> +     * Enable NUMA implicitly for guest to know the maximum memory
> +     * from ACPI SRAT table, which is used for SWIOTLB.
> +     */
> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
> +        if (machine_class->numa_implicit_add_node0) {
> +            machine_class->numa_implicit_add_node0();
> +        }
> +    }

Won't this change guest ABI and so break migration/save/restore ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi Daniel,

At 09/15/2017 04:40 PM, Daniel P. Berrange wrote:
> On Fri, Sep 15, 2017 at 04:33:18PM +0800, Dou Liyang wrote:
>> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT table
>> for transfering NUMA configuration to the guest. So, the maximum memory in
>> SRAT can be used to determine whether to use the swiotlb for IOMMU or not.
>>
>> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
>> never be built. When memory hotplug is enabled, some guest's devices may
>> start failing due to SWIOTLB is disabled.
>>
>> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before QEMU
>> parse NUMA options to enable adding NUMA node implicitly.
>>
>> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Thomas Huth <thuth@redhat.com>
>> Cc: Alistair Francis <alistair23@gmail.com>
>> Cc: f4bug@amsat.org
>> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
>> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
>>
>> ---
>>  hw/i386/pc.c        |  6 ++++++
>>  include/hw/boards.h |  4 ++++
>>  vl.c                | 14 ++++++++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 2108104..3c40117 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2308,6 +2308,11 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
>>      return ms->possible_cpus;
>>  }
>>
>> +static void numa_implicit_add_node0(void)
>> +{
>> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
>> +}
>> +
>>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>  {
>>      /* cpu index isn't used */
>> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
>> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
>>      mc->has_hotpluggable_cpus = true;
>>      mc->default_boot_order = "cad";
>>      mc->hot_add_cpu = pc_hot_add_cpu;
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 7f044d1..898d841 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -141,6 +141,8 @@ typedef struct {
>>   *    should instead use "unimplemented-device" for all memory ranges where
>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>   *    implement and a stub device is required.
>> + * @numa_implicit_add_node0:
>> + *    Enable NUMA implicitly by add a NUMA node.
>>   */
>>  struct MachineClass {
>>      /*< private >*/
>> @@ -191,6 +193,8 @@ struct MachineClass {
>>      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
>>                                                           unsigned cpu_index);
>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>> +
>> +    void (*numa_implicit_add_node0)(void);
>>  };
>>
>>  /**
>> diff --git a/vl.c b/vl.c
>> index fb1f05b..814a5fa 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>>      Error *main_loop_err = NULL;
>>      Error *err = NULL;
>>      bool list_data_dirs = false;
>> +    bool has_numa_config_in_CLI = false;
>>      typedef struct BlockdevOptions_queue {
>>          BlockdevOptions *bdo;
>>          Location loc;
>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>>                  if (!opts) {
>>                      exit(1);
>>                  }
>> +                has_numa_config_in_CLI = true;
>>                  break;
>>              case QEMU_OPTION_display:
>>                  display_type = select_display(optarg);
>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>
>> +    /*
>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
>> +     * NUMA nodes explicitly on CLI
>> +     *
>> +     * Enable NUMA implicitly for guest to know the maximum memory
>> +     * from ACPI SRAT table, which is used for SWIOTLB.
>> +     */
>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
>> +        if (machine_class->numa_implicit_add_node0) {
>> +            machine_class->numa_implicit_add_node0();
>> +        }
>> +    }
>
> Won't this change guest ABI and so break migration/save/restore ?
>
Thank you for your reply, I can't answer this immediately, I will
look into it and reply you soon.

This patch works for X86, has no influence on other arch, and we
can regard it's functionality as "-numa node" in CLI.

Thanks,
	dou.


> Regards,
> Daniel
>

At 09/15/2017 06:05 PM, Dou Liyang wrote:
> Hi Daniel,
>
> At 09/15/2017 04:40 PM, Daniel P. Berrange wrote:
>> On Fri, Sep 15, 2017 at 04:33:18PM +0800, Dou Liyang wrote:
>>> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT
>>> table
>>> for transfering NUMA configuration to the guest. So, the maximum
>>> memory in
>>> SRAT can be used to determine whether to use the swiotlb for IOMMU or
>>> not.
>>>
>>> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
>>> never be built. When memory hotplug is enabled, some guest's devices may
>>> start failing due to SWIOTLB is disabled.
>>>
>>> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before
>>> QEMU
>>> parse NUMA options to enable adding NUMA node implicitly.
>>>
>>> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Thomas Huth <thuth@redhat.com>
>>> Cc: Alistair Francis <alistair23@gmail.com>
>>> Cc: f4bug@amsat.org
>>> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
>>> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
>>>
>>> ---
>>>  hw/i386/pc.c        |  6 ++++++
>>>  include/hw/boards.h |  4 ++++
>>>  vl.c                | 14 ++++++++++++++
>>>  3 files changed, 24 insertions(+)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 2108104..3c40117 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -2308,6 +2308,11 @@ static const CPUArchIdList
>>> *pc_possible_cpu_arch_ids(MachineState *ms)
>>>      return ms->possible_cpus;
>>>  }
>>>
>>> +static void numa_implicit_add_node0(void)
>>> +{
>>> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
>>> +}
>>> +
>>>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>>  {
>>>      /* cpu index isn't used */
>>> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass
>>> *oc, void *data)
>>>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>>>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>>>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
>>> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
>>>      mc->has_hotpluggable_cpus = true;
>>>      mc->default_boot_order = "cad";
>>>      mc->hot_add_cpu = pc_hot_add_cpu;
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index 7f044d1..898d841 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -141,6 +141,8 @@ typedef struct {
>>>   *    should instead use "unimplemented-device" for all memory
>>> ranges where
>>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>>   *    implement and a stub device is required.
>>> + * @numa_implicit_add_node0:
>>> + *    Enable NUMA implicitly by add a NUMA node.
>>>   */
>>>  struct MachineClass {
>>>      /*< private >*/
>>> @@ -191,6 +193,8 @@ struct MachineClass {
>>>      CpuInstanceProperties
>>> (*cpu_index_to_instance_props)(MachineState *machine,
>>>                                                           unsigned
>>> cpu_index);
>>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState
>>> *machine);
>>> +
>>> +    void (*numa_implicit_add_node0)(void);
>>>  };
>>>
>>>  /**
>>> diff --git a/vl.c b/vl.c
>>> index fb1f05b..814a5fa 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>>>      Error *main_loop_err = NULL;
>>>      Error *err = NULL;
>>>      bool list_data_dirs = false;
>>> +    bool has_numa_config_in_CLI = false;
>>>      typedef struct BlockdevOptions_queue {
>>>          BlockdevOptions *bdo;
>>>          Location loc;
>>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>>>                  if (!opts) {
>>>                      exit(1);
>>>                  }
>>> +                has_numa_config_in_CLI = true;
>>>                  break;
>>>              case QEMU_OPTION_display:
>>>                  display_type = select_display(optarg);
>>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>>
>>> +    /*
>>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
>>> +     * NUMA nodes explicitly on CLI
>>> +     *
>>> +     * Enable NUMA implicitly for guest to know the maximum memory
>>> +     * from ACPI SRAT table, which is used for SWIOTLB.
>>> +     */
>>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
>>> +        if (machine_class->numa_implicit_add_node0) {
>>> +            machine_class->numa_implicit_add_node0();
>>> +        }
>>> +    }
>>
>> Won't this change guest ABI

Hi Daniel,

No, this will change the ACPI table in following case without NUMA
configuration:

...
-m 128M,slots=4,maxmem=1G
...

How about fixing it by adding

m->numa_implicit_add_node0 = NULL;

in pc_i440fx/q35_2_10_machine_options() ?


  and so break migration/save/restore ?

I did some tests(save/restore), this don't break migration.

Are some situations I didn't consider ?


Thanks,
	dou.
>>
> Thank you for your reply, I can't answer this immediately, I will
> look into it and reply you soon.
>
> This patch works for X86, has no influence on other arch, and we
> can regard it's functionality as "-numa node" in CLI.
>
> Thanks,
>     dou.
>
>
>> Regards,
>> Daniel
>>

On Mon, 18 Sep 2017 11:54:50 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> At 09/15/2017 06:05 PM, Dou Liyang wrote:
> > Hi Daniel,
> >
> > At 09/15/2017 04:40 PM, Daniel P. Berrange wrote:  
> >> On Fri, Sep 15, 2017 at 04:33:18PM +0800, Dou Liyang wrote:  
> >>> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT
> >>> table
> >>> for transfering NUMA configuration to the guest. So, the maximum
> >>> memory in
> >>> SRAT can be used to determine whether to use the swiotlb for IOMMU or
> >>> not.
> >>>
> >>> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
> >>> never be built. When memory hotplug is enabled, some guest's devices may
> >>> start failing due to SWIOTLB is disabled.
> >>>
> >>> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before
> >>> QEMU
> >>> parse NUMA options to enable adding NUMA node implicitly.
> >>>
> >>> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> >>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Richard Henderson <rth@twiddle.net>
> >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >>> Cc: Marcel Apfelbaum <marcel@redhat.com>
> >>> Cc: Igor Mammedov <imammedo@redhat.com>
> >>> Cc: David Hildenbrand <david@redhat.com>
> >>> Cc: Thomas Huth <thuth@redhat.com>
> >>> Cc: Alistair Francis <alistair23@gmail.com>
> >>> Cc: f4bug@amsat.org
> >>> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
> >>> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
> >>>
> >>> ---
> >>>  hw/i386/pc.c        |  6 ++++++
> >>>  include/hw/boards.h |  4 ++++
> >>>  vl.c                | 14 ++++++++++++++
> >>>  3 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>> index 2108104..3c40117 100644
> >>> --- a/hw/i386/pc.c
> >>> +++ b/hw/i386/pc.c
> >>> @@ -2308,6 +2308,11 @@ static const CPUArchIdList
> >>> *pc_possible_cpu_arch_ids(MachineState *ms)
> >>>      return ms->possible_cpus;
> >>>  }
> >>>
> >>> +static void numa_implicit_add_node0(void)
> >>> +{
> >>> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
> >>> +}
> >>> +
> >>>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
> >>>  {
> >>>      /* cpu index isn't used */
> >>> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass
> >>> *oc, void *data)
> >>>      mc->get_hotplug_handler = pc_get_hotpug_handler;
> >>>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
> >>>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
> >>> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
> >>>      mc->has_hotpluggable_cpus = true;
> >>>      mc->default_boot_order = "cad";
> >>>      mc->hot_add_cpu = pc_hot_add_cpu;
> >>> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >>> index 7f044d1..898d841 100644
> >>> --- a/include/hw/boards.h
> >>> +++ b/include/hw/boards.h
> >>> @@ -141,6 +141,8 @@ typedef struct {
> >>>   *    should instead use "unimplemented-device" for all memory
> >>> ranges where
> >>>   *    the guest will attempt to probe for a device that QEMU doesn't
> >>>   *    implement and a stub device is required.
> >>> + * @numa_implicit_add_node0:
> >>> + *    Enable NUMA implicitly by add a NUMA node.
> >>>   */
> >>>  struct MachineClass {
> >>>      /*< private >*/
> >>> @@ -191,6 +193,8 @@ struct MachineClass {
> >>>      CpuInstanceProperties
> >>> (*cpu_index_to_instance_props)(MachineState *machine,
> >>>                                                           unsigned
> >>> cpu_index);
> >>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState
> >>> *machine);
> >>> +
> >>> +    void (*numa_implicit_add_node0)(void);
> >>>  };
> >>>
> >>>  /**
> >>> diff --git a/vl.c b/vl.c
> >>> index fb1f05b..814a5fa 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
> >>>      Error *main_loop_err = NULL;
> >>>      Error *err = NULL;
> >>>      bool list_data_dirs = false;
> >>> +    bool has_numa_config_in_CLI = false;
> >>>      typedef struct BlockdevOptions_queue {
> >>>          BlockdevOptions *bdo;
> >>>          Location loc;
> >>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
> >>>                  if (!opts) {
> >>>                      exit(1);
> >>>                  }
> >>> +                has_numa_config_in_CLI = true;
> >>>                  break;
> >>>              case QEMU_OPTION_display:
> >>>                  display_type = select_display(optarg);
> >>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
> >>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> >>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> >>>
> >>> +    /*
> >>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
> >>> +     * NUMA nodes explicitly on CLI
> >>> +     *
> >>> +     * Enable NUMA implicitly for guest to know the maximum memory
> >>> +     * from ACPI SRAT table, which is used for SWIOTLB.
> >>> +     */
> >>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
> >>> +        if (machine_class->numa_implicit_add_node0) {
> >>> +            machine_class->numa_implicit_add_node0();
> >>> +        }
> >>> +    }  
> >>
> >> Won't this change guest ABI  
> 
> Hi Daniel,
> 
> No, this will change the ACPI table in following case without NUMA
> configuration:
it also will affect FW_CFG_NUMA file length (read as break ABI),
you didn't see any breakage because currnet SeaBIOS don't use
this legacy file anymore and even if it would used it's still
a race one has to at boot.

So you need to disable feature on old machine types as you've suggested

> 
> ...
> -m 128M,slots=4,maxmem=1G
> ...
> 
> How about fixing it by adding
> 
> m->numa_implicit_add_node0 = NULL;
> 
> in pc_i440fx/q35_2_10_machine_options() ?
yep, pls do so.
Also take a look spapr which also supports numa, to make sure
it won't regress.
CCing David.

> 
> 
>   and so break migration/save/restore ?
> 
> I did some tests(save/restore), this don't break migration.
> 
> Are some situations I didn't consider ?
> 
> 
> Thanks,
> 	dou.
> >>  
> > Thank you for your reply, I can't answer this immediately, I will
> > look into it and reply you soon.
> >
> > This patch works for X86, has no influence on other arch, and we
> > can regard it's functionality as "-numa node" in CLI.
> >
> > Thanks,
> >     dou.
> >
> >  
> >> Regards,
> >> Daniel
> >>  
> 
> 
> 

At 09/18/2017 03:40 PM, Igor Mammedov wrote:
> On Mon, 18 Sep 2017 11:54:50 +0800
> Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
>
>> At 09/15/2017 06:05 PM, Dou Liyang wrote:
>>> Hi Daniel,
>>>
>>> At 09/15/2017 04:40 PM, Daniel P. Berrange wrote:
>>>> On Fri, Sep 15, 2017 at 04:33:18PM +0800, Dou Liyang wrote:
>>>>> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT
>>>>> table
>>>>> for transfering NUMA configuration to the guest. So, the maximum
>>>>> memory in
>>>>> SRAT can be used to determine whether to use the swiotlb for IOMMU or
>>>>> not.
>>>>>
>>>>> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
>>>>> never be built. When memory hotplug is enabled, some guest's devices may
>>>>> start failing due to SWIOTLB is disabled.
>>>>>
>>>>> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before
>>>>> QEMU
>>>>> parse NUMA options to enable adding NUMA node implicitly.
>>>>>
>>>>> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>>>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>>>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>> Cc: Thomas Huth <thuth@redhat.com>
>>>>> Cc: Alistair Francis <alistair23@gmail.com>
>>>>> Cc: f4bug@amsat.org
>>>>> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
>>>>> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
>>>>>
>>>>> ---
>>>>>  hw/i386/pc.c        |  6 ++++++
>>>>>  include/hw/boards.h |  4 ++++
>>>>>  vl.c                | 14 ++++++++++++++
>>>>>  3 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>> index 2108104..3c40117 100644
>>>>> --- a/hw/i386/pc.c
>>>>> +++ b/hw/i386/pc.c
>>>>> @@ -2308,6 +2308,11 @@ static const CPUArchIdList
>>>>> *pc_possible_cpu_arch_ids(MachineState *ms)
>>>>>      return ms->possible_cpus;
>>>>>  }
>>>>>
>>>>> +static void numa_implicit_add_node0(void)
>>>>> +{
>>>>> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
>>>>> +}
>>>>> +
>>>>>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>>>>  {
>>>>>      /* cpu index isn't used */
>>>>> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass
>>>>> *oc, void *data)
>>>>>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>>>>>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>>>>>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
>>>>> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
>>>>>      mc->has_hotpluggable_cpus = true;
>>>>>      mc->default_boot_order = "cad";
>>>>>      mc->hot_add_cpu = pc_hot_add_cpu;
>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>>> index 7f044d1..898d841 100644
>>>>> --- a/include/hw/boards.h
>>>>> +++ b/include/hw/boards.h
>>>>> @@ -141,6 +141,8 @@ typedef struct {
>>>>>   *    should instead use "unimplemented-device" for all memory
>>>>> ranges where
>>>>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>>>>   *    implement and a stub device is required.
>>>>> + * @numa_implicit_add_node0:
>>>>> + *    Enable NUMA implicitly by add a NUMA node.
>>>>>   */
>>>>>  struct MachineClass {
>>>>>      /*< private >*/
>>>>> @@ -191,6 +193,8 @@ struct MachineClass {
>>>>>      CpuInstanceProperties
>>>>> (*cpu_index_to_instance_props)(MachineState *machine,
>>>>>                                                           unsigned
>>>>> cpu_index);
>>>>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState
>>>>> *machine);
>>>>> +
>>>>> +    void (*numa_implicit_add_node0)(void);
>>>>>  };
>>>>>
>>>>>  /**
>>>>> diff --git a/vl.c b/vl.c
>>>>> index fb1f05b..814a5fa 100644
>>>>> --- a/vl.c
>>>>> +++ b/vl.c
>>>>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>>>>>      Error *main_loop_err = NULL;
>>>>>      Error *err = NULL;
>>>>>      bool list_data_dirs = false;
>>>>> +    bool has_numa_config_in_CLI = false;
>>>>>      typedef struct BlockdevOptions_queue {
>>>>>          BlockdevOptions *bdo;
>>>>>          Location loc;
>>>>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>>>>>                  if (!opts) {
>>>>>                      exit(1);
>>>>>                  }
>>>>> +                has_numa_config_in_CLI = true;
>>>>>                  break;
>>>>>              case QEMU_OPTION_display:
>>>>>                  display_type = select_display(optarg);
>>>>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>>>>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>>>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>>>>
>>>>> +    /*
>>>>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
>>>>> +     * NUMA nodes explicitly on CLI
>>>>> +     *
>>>>> +     * Enable NUMA implicitly for guest to know the maximum memory
>>>>> +     * from ACPI SRAT table, which is used for SWIOTLB.
>>>>> +     */
>>>>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
>>>>> +        if (machine_class->numa_implicit_add_node0) {
>>>>> +            machine_class->numa_implicit_add_node0();
>>>>> +        }
>>>>> +    }
>>>>
>>>> Won't this change guest ABI
>>
>> Hi Daniel,
>>
>> No, this will change the ACPI table in following case without NUMA
>> configuration:
> it also will affect FW_CFG_NUMA file length (read as break ABI),
> you didn't see any breakage because currnet SeaBIOS don't use
> this legacy file anymore and even if it would used it's still
> a race one has to at boot.
>
> So you need to disable feature on old machine types as you've suggested
>

Hi Igor,

Thank you very much. yes, I will.

>>
>> ...
>> -m 128M,slots=4,maxmem=1G
>> ...
>>
>> How about fixing it by adding
>>
>> m->numa_implicit_add_node0 = NULL;
>>
>> in pc_i440fx/q35_2_10_machine_options() ?
> yep, pls do so.
> Also take a look spapr which also supports numa, to make sure
> it won't regress.

Currently, just setup the pointer of pc_machine_class_init in
pc_machine_class_init() for X86 and it's NULL in other arches. So there
is no influence on SPAPR.

And this bug only affects x86, seems no need to add NUMA node implicity
for SPAPR as well.

Thanks,
	dou.

> CCing David.
>
>>
>>
>>   and so break migration/save/restore ?
>>
>> I did some tests(save/restore), this don't break migration.
>>
>> Are some situations I didn't consider ?
>>
>>
>> Thanks,
>> 	dou.
>>>>
>>> Thank you for your reply, I can't answer this immediately, I will
>>> look into it and reply you soon.
>>>
>>> This patch works for X86, has no influence on other arch, and we
>>> can regard it's functionality as "-numa node" in CLI.
>>>
>>> Thanks,
>>>     dou.
>>>
>>>
>>>> Regards,
>>>> Daniel
>>>>
>>
>>
>>
>
>
>
>

On Fri, 15 Sep 2017 16:33:18 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT table
> for transfering NUMA configuration to the guest. So, the maximum memory in
> SRAT can be used to determine whether to use the swiotlb for IOMMU or not.
> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
> never be built. When memory hotplug is enabled, some guest's devices may
> start failing due to SWIOTLB is disabled.
> 
> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before QEMU
> parse NUMA options to enable adding NUMA node implicitly.


I'd rewrite commit message with something like:

Linux and Windows need ACPI SRAT table to make memory
hotplug work properly, however currently QEMU doesn't
create SRAT table if numa options aren't present on CLI.

Which breaks both linux and windows guests in certain
conditions:
 * windows: won't enable memory hotplug without SRAT table at all
 * linux: if QEMU is started with initial memory all below
   4Gb and no SRAT table present, guest kernel will use
   nommu DMA ops, which breaks 32bit hw drivers when memory
   is hotplugged and guest tries to use it with that drivers.

Fix above issues by automatically creating a numa node when
QEMU is started with memory hotplug enabled but without '-numa'
options on CLI.
(PS: auto-create numa node only for new machine types so
not to break migration).

Which would provide SRAT table to guests without explicit
-numa options on CLI and would allow:
 * windows: to enable memory hotplug
 * linux: switch to SWIOTLB DMA ops, to bounce DMA transfers
   to 32bit allocated buffers that legacy drivers/hw can handle.


> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Alistair Francis <alistair23@gmail.com>
> Cc: f4bug@amsat.org
> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
> 
> ---
>  hw/i386/pc.c        |  6 ++++++
>  include/hw/boards.h |  4 ++++
>  vl.c                | 14 ++++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2108104..3c40117 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2308,6 +2308,11 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
>      return ms->possible_cpus;
>  }
>  
> +static void numa_implicit_add_node0(void)
> +{
> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
> +}
> +
>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>  {
>      /* cpu index isn't used */
> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
>      mc->has_hotpluggable_cpus = true;
>      mc->default_boot_order = "cad";
>      mc->hot_add_cpu = pc_hot_add_cpu;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7f044d1..898d841 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -141,6 +141,8 @@ typedef struct {
>   *    should instead use "unimplemented-device" for all memory ranges where
>   *    the guest will attempt to probe for a device that QEMU doesn't
>   *    implement and a stub device is required.
> + * @numa_implicit_add_node0:
> + *    Enable NUMA implicitly by add a NUMA node.
how about:
s/auto_enable_numa_with_memhp/
boolean instead, see below how it could improve patch.

>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -191,6 +193,8 @@ struct MachineClass {
>      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
>                                                           unsigned cpu_index);
>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> +
> +    void (*numa_implicit_add_node0)(void);
>  };
>  
>  /**
> diff --git a/vl.c b/vl.c
> index fb1f05b..814a5fa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>      Error *main_loop_err = NULL;
>      Error *err = NULL;
>      bool list_data_dirs = false;
> +    bool has_numa_config_in_CLI = false;
>      typedef struct BlockdevOptions_queue {
>          BlockdevOptions *bdo;
>          Location loc;
> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>                  if (!opts) {
>                      exit(1);
>                  }
> +                has_numa_config_in_CLI = true;
>                  break;
>              case QEMU_OPTION_display:
>                  display_type = select_display(optarg);
> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>  
> +    /*
> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
> +     * NUMA nodes explicitly on CLI
> +     *
> +     * Enable NUMA implicitly for guest to know the maximum memory
> +     * from ACPI SRAT table, which is used for SWIOTLB.
> +     */
> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
> +        if (machine_class->numa_implicit_add_node0) {
> +            machine_class->numa_implicit_add_node0();
> +        }
> +    }
>      parse_numa_opts(current_machine);
it would be better to put this logic inside of parse_numa_opts()
I'd suggest to move:

    current_machine->ram_size = ram_size;                                        
    current_machine->maxram_size = maxram_size;                                  
    current_machine->ram_slots = ram_slots;

before parse_numa_opts() is called, and then
handle 'memhp present+no numa on CLI" logic inside of
parse_numa_opts(). With this you won't have to track
'has_numa_config_in_CLI', drop callback numa_implicit_add_node0()
and numa nuances would be in place they are supposed to be: numa.c

>  
>      if (qemu_opts_foreach(qemu_find_opts("mon"),

Hi Igor,

At 09/18/2017 05:08 PM, Igor Mammedov wrote:
> On Fri, 15 Sep 2017 16:33:18 +0800
> Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
>
>> In QEMU, if we enable NUMA and have nodes, QEMU will build ACPI SRAT table
>> for transfering NUMA configuration to the guest. So, the maximum memory in
>> SRAT can be used to determine whether to use the swiotlb for IOMMU or not.
>> However, if QEmu doesn't enable NUMA explicitly on CLI, The SRAT will
>> never be built. When memory hotplug is enabled, some guest's devices may
>> start failing due to SWIOTLB is disabled.
>>
>> Add numa_implicit_add_node0 in struct MachineClass, Invoke it before QEMU
>> parse NUMA options to enable adding NUMA node implicitly.
>
>
> I'd rewrite commit message with something like:
>
> Linux and Windows need ACPI SRAT table to make memory
> hotplug work properly, however currently QEMU doesn't
> create SRAT table if numa options aren't present on CLI.
>
> Which breaks both linux and windows guests in certain
> conditions:
>  * windows: won't enable memory hotplug without SRAT table at all
>  * linux: if QEMU is started with initial memory all below
>    4Gb and no SRAT table present, guest kernel will use
>    nommu DMA ops, which breaks 32bit hw drivers when memory
>    is hotplugged and guest tries to use it with that drivers.
>
> Fix above issues by automatically creating a numa node when
> QEMU is started with memory hotplug enabled but without '-numa'
> options on CLI.
> (PS: auto-create numa node only for new machine types so
> not to break migration).
>
> Which would provide SRAT table to guests without explicit
> -numa options on CLI and would allow:
>  * windows: to enable memory hotplug
>  * linux: switch to SWIOTLB DMA ops, to bounce DMA transfers
>    to 32bit allocated buffers that legacy drivers/hw can handle.
>

It's more detailed and clear to understand, I will update the changelog
in the next version.

Thanks,
	dou.
>
>> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Thomas Huth <thuth@redhat.com>
>> Cc: Alistair Francis <alistair23@gmail.com>
>> Cc: f4bug@amsat.org
>> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
>> Cc: Izumi Taku <izumi.taku@jp.fujitsu.com>
>>
>> ---
>>  hw/i386/pc.c        |  6 ++++++
>>  include/hw/boards.h |  4 ++++
>>  vl.c                | 14 ++++++++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 2108104..3c40117 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2308,6 +2308,11 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
>>      return ms->possible_cpus;
>>  }
>>
>> +static void numa_implicit_add_node0(void)
>> +{
>> +    qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
>> +}
>> +
>>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>  {
>>      /* cpu index isn't used */
>> @@ -2349,6 +2354,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
>> +    mc->numa_implicit_add_node0 = numa_implicit_add_node0;
>>      mc->has_hotpluggable_cpus = true;
>>      mc->default_boot_order = "cad";
>>      mc->hot_add_cpu = pc_hot_add_cpu;
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 7f044d1..898d841 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -141,6 +141,8 @@ typedef struct {
>>   *    should instead use "unimplemented-device" for all memory ranges where
>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>   *    implement and a stub device is required.
>> + * @numa_implicit_add_node0:
>> + *    Enable NUMA implicitly by add a NUMA node.
> how about:
> s/auto_enable_numa_with_memhp/
> boolean instead, see below how it could improve patch.
>
>>   */
>>  struct MachineClass {
>>      /*< private >*/
>> @@ -191,6 +193,8 @@ struct MachineClass {
>>      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
>>                                                           unsigned cpu_index);
>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>> +
>> +    void (*numa_implicit_add_node0)(void);
>>  };
>>
>>  /**
>> diff --git a/vl.c b/vl.c
>> index fb1f05b..814a5fa 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>>      Error *main_loop_err = NULL;
>>      Error *err = NULL;
>>      bool list_data_dirs = false;
>> +    bool has_numa_config_in_CLI = false;
>>      typedef struct BlockdevOptions_queue {
>>          BlockdevOptions *bdo;
>>          Location loc;
>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>>                  if (!opts) {
>>                      exit(1);
>>                  }
>> +                has_numa_config_in_CLI = true;
>>                  break;
>>              case QEMU_OPTION_display:
>>                  display_type = select_display(optarg);
>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>
>> +    /*
>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
>> +     * NUMA nodes explicitly on CLI
>> +     *
>> +     * Enable NUMA implicitly for guest to know the maximum memory
>> +     * from ACPI SRAT table, which is used for SWIOTLB.
>> +     */
>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
>> +        if (machine_class->numa_implicit_add_node0) {
>> +            machine_class->numa_implicit_add_node0();
>> +        }
>> +    }
>>      parse_numa_opts(current_machine);
> it would be better to put this logic inside of parse_numa_opts()
> I'd suggest to move:
>
>     current_machine->ram_size = ram_size;
>     current_machine->maxram_size = maxram_size;
>     current_machine->ram_slots = ram_slots;
>
> before parse_numa_opts() is called, and then
> handle 'memhp present+no numa on CLI" logic inside of
> parse_numa_opts(). With this you won't have to track
> 'has_numa_config_in_CLI', drop callback numa_implicit_add_node0()
> and numa nuances would be in place they are supposed to be: numa.c
>
>>
>>      if (qemu_opts_foreach(qemu_find_opts("mon"),
>
>
>
>

Hi Igor,

I am sorry I missed some comments you gave to me.

my reply is below.
At 09/18/2017 05:24 PM, Dou Liyang wrote:
[...]
>>> ranges where
>>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>>   *    implement and a stub device is required.
>>> + * @numa_implicit_add_node0:
>>> + *    Enable NUMA implicitly by add a NUMA node.
>> how about:
>> s/auto_enable_numa_with_memhp/

Yes, really better than me, will do it.

>> boolean instead, see below how it could improve patch.
>>

I am not really sure why do we want to make this function boolean.

>>>   */
>>>  struct MachineClass {
>>>      /*< private >*/
>>> @@ -191,6 +193,8 @@ struct MachineClass {
>>>      CpuInstanceProperties
>>> (*cpu_index_to_instance_props)(MachineState *machine,
>>>                                                           unsigned
>>> cpu_index);
>>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState
>>> *machine);
>>> +
>>> +    void (*numa_implicit_add_node0)(void);
>>>  };
>>>
>>>  /**
>>> diff --git a/vl.c b/vl.c
>>> index fb1f05b..814a5fa 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
>>>      Error *main_loop_err = NULL;
>>>      Error *err = NULL;
>>>      bool list_data_dirs = false;
>>> +    bool has_numa_config_in_CLI = false;
>>>      typedef struct BlockdevOptions_queue {
>>>          BlockdevOptions *bdo;
>>>          Location loc;
>>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
>>>                  if (!opts) {
>>>                      exit(1);
>>>                  }
>>> +                has_numa_config_in_CLI = true;
>>>                  break;
>>>              case QEMU_OPTION_display:
>>>                  display_type = select_display(optarg);
>>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
>>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>>
>>> +    /*
>>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
>>> +     * NUMA nodes explicitly on CLI
>>> +     *
>>> +     * Enable NUMA implicitly for guest to know the maximum memory
>>> +     * from ACPI SRAT table, which is used for SWIOTLB.
>>> +     */
>>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
>>> +        if (machine_class->numa_implicit_add_node0) {
>>> +            machine_class->numa_implicit_add_node0();
>>> +        }
>>> +    }
>>>      parse_numa_opts(current_machine);
>> it would be better to put this logic inside of parse_numa_opts()
>> I'd suggest to move:
>>
>>     current_machine->ram_size = ram_size;
>>     current_machine->maxram_size = maxram_size;
>>     current_machine->ram_slots = ram_slots;
>>
>> before parse_numa_opts() is called, and then
>> handle 'memhp present+no numa on CLI" logic inside of
>> parse_numa_opts(). With this you won't have to track
>> 'has_numa_config_in_CLI', drop callback numa_implicit_add_node0()
>> and numa nuances would be in place they are supposed to be: numa.c
>>

Is "dropping the callback..." means :

     static void auto_enable_numa_with_memhp(QemuOptsList *list)
     {
         ...
     }

     void parse_numa_opts(MachineState *ms, uint64_t ram_slots)
     {
         QemuOptsList *numa_opts = qemu_find_opts("numa");
         ...
         auto_enable_numa_with_memhp(numa_opts);
         ...
     }

So, No matter what arch it is, if it support NUMA, we will enable NUMA
implicitly when it has already enabled memory hotplug by 
"slot=xx,maxmem=xx" CLI explicitly.

I am not sure that, but this bug only affects x86 as I know, seems no
need to affect other arches which support NUMA as well.

Thanks,
	dou.
>>>
>>>      if (qemu_opts_foreach(qemu_find_opts("mon"),
>>
>>
>>
>>

On Thu, 21 Sep 2017 12:19:17 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> 
> I am sorry I missed some comments you gave to me.
> 
> my reply is below.
> At 09/18/2017 05:24 PM, Dou Liyang wrote:
> [...]
> >>> ranges where
> >>>   *    the guest will attempt to probe for a device that QEMU doesn't
> >>>   *    implement and a stub device is required.
> >>> + * @numa_implicit_add_node0:
> >>> + *    Enable NUMA implicitly by add a NUMA node.  
> >> how about:
> >> s/auto_enable_numa_with_memhp/  
> 
> Yes, really better than me, will do it.
> 
> >> boolean instead, see below how it could improve patch.
> >>  
> 
> I am not really sure why do we want to make this function boolean.
> 
> >>>   */
> >>>  struct MachineClass {
> >>>      /*< private >*/
> >>> @@ -191,6 +193,8 @@ struct MachineClass {
> >>>      CpuInstanceProperties
> >>> (*cpu_index_to_instance_props)(MachineState *machine,
> >>>                                                           unsigned
> >>> cpu_index);
> >>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState
> >>> *machine);
> >>> +
> >>> +    void (*numa_implicit_add_node0)(void);
> >>>  };
> >>>
> >>>  /**
> >>> diff --git a/vl.c b/vl.c
> >>> index fb1f05b..814a5fa 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
> >>>      Error *main_loop_err = NULL;
> >>>      Error *err = NULL;
> >>>      bool list_data_dirs = false;
> >>> +    bool has_numa_config_in_CLI = false;
> >>>      typedef struct BlockdevOptions_queue {
> >>>          BlockdevOptions *bdo;
> >>>          Location loc;
> >>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
> >>>                  if (!opts) {
> >>>                      exit(1);
> >>>                  }
> >>> +                has_numa_config_in_CLI = true;
> >>>                  break;
> >>>              case QEMU_OPTION_display:
> >>>                  display_type = select_display(optarg);
> >>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
> >>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> >>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> >>>
> >>> +    /*
> >>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
> >>> +     * NUMA nodes explicitly on CLI
> >>> +     *
> >>> +     * Enable NUMA implicitly for guest to know the maximum memory
> >>> +     * from ACPI SRAT table, which is used for SWIOTLB.
> >>> +     */
> >>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
> >>> +        if (machine_class->numa_implicit_add_node0) {
> >>> +            machine_class->numa_implicit_add_node0();
> >>> +        }
> >>> +    }
> >>>      parse_numa_opts(current_machine);  
> >> it would be better to put this logic inside of parse_numa_opts()
> >> I'd suggest to move:
> >>
> >>     current_machine->ram_size = ram_size;
> >>     current_machine->maxram_size = maxram_size;
> >>     current_machine->ram_slots = ram_slots;
> >>
> >> before parse_numa_opts() is called, and then
> >> handle 'memhp present+no numa on CLI" logic inside of
> >> parse_numa_opts(). With this you won't have to track
> >> 'has_numa_config_in_CLI', drop callback numa_implicit_add_node0()
> >> and numa nuances would be in place they are supposed to be: numa.c
> >>  
> 
> Is "dropping the callback..." means :
> 
>      static void auto_enable_numa_with_memhp(QemuOptsList *list)
>      {
>          ...
>      }
> 
>      void parse_numa_opts(MachineState *ms, uint64_t ram_slots)
>      {
>          QemuOptsList *numa_opts = qemu_find_opts("numa");
>          ...
>          auto_enable_numa_with_memhp(numa_opts);
>          ...
>      }
> 
> So, No matter what arch it is, if it support NUMA, we will enable NUMA
> implicitly when it has already enabled memory hotplug by 
> "slot=xx,maxmem=xx" CLI explicitly.
> 
> I am not sure that, but this bug only affects x86 as I know, seems no
> need to affect other arches which support NUMA as well.

I've meant something like that:

parse_numa_opts() {
    if (mc->auto_enable_numa_with_memhp == true) {
        qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
    }
}

where x86 sets mc->auto_enable_numa_with_memhp to true by default
and sets it to false for 2.10 and older machine types.
It will allow individual machines to enable feature if they need it
but prevent guest ABI breakage for old machine types
(i.e. won't break migration)

grep source for 'pcmc->legacy_cpu_hotplug = true' to see how
this compat stuff works.

> 
> Thanks,
> 	dou.
> >>>
> >>>      if (qemu_opts_foreach(qemu_find_opts("mon"),  
> >>
> >>
> >>
> >>  
> 
> 

Hi Igor,

At 09/21/2017 03:54 PM, Igor Mammedov wrote:
> On Thu, 21 Sep 2017 12:19:17 +0800

[...]

>
> I've meant something like that:
>
> parse_numa_opts() {
>     if (mc->auto_enable_numa_with_memhp == true) {
>         qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
>     }
> }
>

Oops, Understood!  ;-) .

> where x86 sets mc->auto_enable_numa_with_memhp to true by default
> and sets it to false for 2.10 and older machine types.
> It will allow individual machines to enable feature if they need it
> but prevent guest ABI breakage for old machine types
> (i.e. won't break migration)
>

Got it.

> grep source for 'pcmc->legacy_cpu_hotplug = true' to see how
> this compact stuff works.
>

OK. Thank you very much.

Thanks,
	dou