[Qemu-devel] [PATCH 0/2] fix two bugs about numa _and_ hotplug memory feature

[Qemu-devel] [PATCH 0/2] fix two bugs about numa _and_ hotplug memory feature

[zhanghailiang]

Hi,

In qemu, We do not update the numa node memory size after hotplug memory,
There will be a confused result for hmp command "info numa" after hotplug memory.
Also, if we do not configure numa option, we can't hotplug memory, i think this is
a bug.

This patch series fix them.


zhanghailiang (2):
  pc-dimm: No numa option shouldn't break hotplug memory feature
  numa/pc-dimm: Fix stat of memory size in node when hotplug memory

 hw/i386/pc.c            |  1 +
 hw/mem/pc-dimm.c        |  3 +-
 include/sysemu/sysemu.h |  1 +
 numa.c                  | 13 +++++++++++++
 4 files changed, 16 insertions(+), 1 deletion(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[zhanghailiang]

If we do not configure numa option, memory hotplug should work as well.
It should not depend on numa option.

Steps to reproduce:
(1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
(2) Hotplug memory
It will fail and reports:
"'DIMM property node has value 0' which exceeds the number of numa nodes: 0"

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/mem/pc-dimm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 5bfc5b7..a800ea7 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -252,7 +252,7 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
         return;
     }
-    if (dimm->node >= nb_numa_nodes) {
+    if ((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) {
         error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
                    PRIu32 "' which exceeds the number of numa nodes: %d",
                    dimm->node, nb_numa_nodes);
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/2] numa/pc-dimm: Fix stat of memory size in node when hotplug memory

[zhanghailiang]

When do memory hotplug, if there is numa node, we should add
the memory size to the corresponding node memory size.

For now, it mainly affects the result of hmp command "info numa".

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/i386/pc.c            |  1 +
 include/sysemu/sysemu.h |  1 +
 numa.c                  | 13 +++++++++++++
 3 files changed, 15 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 77b6782..bafe48e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1607,6 +1607,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     memory_region_add_subregion(&pcms->hotplug_memory,
                                 addr - pcms->hotplug_memory_base, mr);
     vmstate_register_ram(mr, dev);
+    update_numa_node_mem_size(dimm->node, memory_region_size(mr), true);
 
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d8539fd..5bc9d73 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -160,6 +160,7 @@ typedef struct node_info {
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);
 void set_numa_modes(void);
+void update_numa_node_mem_size(int node, uint64_t size, bool hotplug);
 extern QemuOptsList qemu_numa_opts;
 int numa_init_func(QemuOpts *opts, void *opaque);
 
diff --git a/numa.c b/numa.c
index 3b98135..bbfa16f 100644
--- a/numa.c
+++ b/numa.c
@@ -315,6 +315,19 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
     }
 }
 
+void update_numa_node_mem_size(int node, uint64_t size, bool hotplug)
+{
+    if (!nb_numa_nodes) {
+        return;
+    }
+
+    if (hotplug) {
+        numa_info[node].node_mem += size;
+    } else {
+        numa_info[node].node_mem -= size;
+    }
+}
+
 static int query_memdev(Object *obj, void *opaque)
 {
     MemdevList **list = opaque;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/2] numa/pc-dimm: Fix stat of memory size in node when hotplug memory

[Igor Mammedov]

On Tue, 16 Sep 2014 18:39:16 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> When do memory hotplug, if there is numa node, we should add
> the memory size to the corresponding node memory size.
> 
> For now, it mainly affects the result of hmp command "info numa".
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/i386/pc.c            |  1 +
>  include/sysemu/sysemu.h |  1 +
>  numa.c                  | 13 +++++++++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 77b6782..bafe48e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1607,6 +1607,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      memory_region_add_subregion(&pcms->hotplug_memory,
>                                  addr - pcms->hotplug_memory_base, mr);
>      vmstate_register_ram(mr, dev);
> +    update_numa_node_mem_size(dimm->node, memory_region_size(mr), true);
>  
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index d8539fd..5bc9d73 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -160,6 +160,7 @@ typedef struct node_info {
>  extern NodeInfo numa_info[MAX_NODES];
>  void set_numa_nodes(void);
>  void set_numa_modes(void);
> +void update_numa_node_mem_size(int node, uint64_t size, bool hotplug);
>  extern QemuOptsList qemu_numa_opts;
>  int numa_init_func(QemuOpts *opts, void *opaque);
>  
> diff --git a/numa.c b/numa.c
> index 3b98135..bbfa16f 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -315,6 +315,19 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>      }
>  }
>  
> +void update_numa_node_mem_size(int node, uint64_t size, bool hotplug)
> +{
> +    if (!nb_numa_nodes) {
> +        return;
> +    }
> +
> +    if (hotplug) {
> +        numa_info[node].node_mem += size;
> +    } else {
> +        numa_info[node].node_mem -= size;
> +    }
> +}
I don't think is right thing to do.
numa_info.node_mem provides mapping of initial memory specified with -m XXX option,
and later used for building ACPI SRAT table.

I'd suggest to fix 'info numa' command instead. Add to it ability to enumerate
all memory devices, in that case when initial memory is converted to memory devices
'info numa' will continue to work correctly.

> +
>  static int query_memdev(Object *obj, void *opaque)
>  {
>      MemdevList **list = opaque;

Re: [Qemu-devel] [PATCH 2/2] numa/pc-dimm: Fix stat of memory size in node when hotplug memory

[zhanghailiang]

On 2014/9/16 19:20, Igor Mammedov wrote:
> On Tue, 16 Sep 2014 18:39:16 +0800
> zhanghailiang<zhang.zhanghailiang@huawei.com>  wrote:
>
>> When do memory hotplug, if there is numa node, we should add
>> the memory size to the corresponding node memory size.
>>
>> For now, it mainly affects the result of hmp command "info numa".
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>   hw/i386/pc.c            |  1 +
>>   include/sysemu/sysemu.h |  1 +
>>   numa.c                  | 13 +++++++++++++
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 77b6782..bafe48e 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1607,6 +1607,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>       memory_region_add_subregion(&pcms->hotplug_memory,
>>                                   addr - pcms->hotplug_memory_base, mr);
>>       vmstate_register_ram(mr, dev);
>> +    update_numa_node_mem_size(dimm->node, memory_region_size(mr), true);
>>
>>       hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>       hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev,&local_err);
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index d8539fd..5bc9d73 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -160,6 +160,7 @@ typedef struct node_info {
>>   extern NodeInfo numa_info[MAX_NODES];
>>   void set_numa_nodes(void);
>>   void set_numa_modes(void);
>> +void update_numa_node_mem_size(int node, uint64_t size, bool hotplug);
>>   extern QemuOptsList qemu_numa_opts;
>>   int numa_init_func(QemuOpts *opts, void *opaque);
>>
>> diff --git a/numa.c b/numa.c
>> index 3b98135..bbfa16f 100644
>> --- a/numa.c
>> +++ b/numa.c
>> @@ -315,6 +315,19 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>>       }
>>   }
>>
>> +void update_numa_node_mem_size(int node, uint64_t size, bool hotplug)
>> +{
>> +    if (!nb_numa_nodes) {
>> +        return;
>> +    }
>> +
>> +    if (hotplug) {
>> +        numa_info[node].node_mem += size;
>> +    } else {
>> +        numa_info[node].node_mem -= size;
>> +    }
>> +}
> I don't think is right thing to do.
> numa_info.node_mem provides mapping of initial memory specified with -m XXX option,
> and later used for building ACPI SRAT table.
>

Got it;)

> I'd suggest to fix 'info numa' command instead. Add to it ability to enumerate
> all memory devices, in that case when initial memory is converted to memory devices
> 'info numa' will continue to work correctly.
>

Hmm, Good idea, i will fix it.Thanks.

>> +
>>   static int query_memdev(Object *obj, void *opaque)
>>   {
>>       MemdevList **list = opaque;
>
>
> .
>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Hu Tao]

On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
> If we do not configure numa option, memory hotplug should work as well.
> It should not depend on numa option.
> 
> Steps to reproduce:
> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> (2) Hotplug memory
> It will fail and reports:
> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> 

I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:

  I thnk that there should be no
  cases when dimm is plugged (and check from patch is fired up) without
  actually populated NUMA, because not every OS will workaround this by
  faking the node.

https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html

Have you tested this patch with Windows guest?

Regards,
Hu

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[zhanghailiang]

On 2014/9/17 16:32, Hu Tao wrote:
> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
>> If we do not configure numa option, memory hotplug should work as well.
>> It should not depend on numa option.
>>
>> Steps to reproduce:
>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
>> (2) Hotplug memory
>> It will fail and reports:
>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
>>
>
> I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:
>
>    I thnk that there should be no
>    cases when dimm is plugged (and check from patch is fired up) without
>    actually populated NUMA, because not every OS will workaround this by
>    faking the node.
>
> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
>
> Have you tested this patch with Windows guest?
>

Hmm, I have just tested this, and Yes, it didn't work for Windows guest.
Thanks for your kind reminder.;)

Best regards,
zhanghailiang

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Tang Chen]

Add Andrey Korolyov <andrey@xdel.ru>

On 09/17/2014 04:32 PM, Hu Tao wrote:
> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
>> If we do not configure numa option, memory hotplug should work as well.
>> It should not depend on numa option.
>>
>> Steps to reproduce:
>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
>> (2) Hotplug memory
>> It will fail and reports:
>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
>>
> I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:
>
>    I thnk that there should be no
>    cases when dimm is plugged (and check from patch is fired up) without
>    actually populated NUMA, because not every OS will workaround this by
>    faking the node.

According to Andrey Korolyov <andrey@xdel.ru>, memory hotplug should not
work without SRAT. So maybe forcing to create a NUMA node and SRAT will be
better idea.

I'm have been working on it.

Thanks.

>
> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
>
> Have you tested this patch with Windows guest?
>
> Regards,
> Hu
>
>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Andrey Korolyov]

On Wed, Sep 17, 2014 at 2:00 PM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> Add Andrey Korolyov <andrey@xdel.ru>
>
> On 09/17/2014 04:32 PM, Hu Tao wrote:
>>
>> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
>>>
>>> If we do not configure numa option, memory hotplug should work as well.
>>> It should not depend on numa option.
>>>
>>> Steps to reproduce:
>>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
>>> (2) Hotplug memory
>>> It will fail and reports:
>>> "'DIMM property node has value 0' which exceeds the number of numa nodes:
>>> 0"
>>>
>> I rememberd Tang Chen had a patch for this bug, this is what Andrey
>> suggested:
>>
>>    I thnk that there should be no
>>    cases when dimm is plugged (and check from patch is fired up) without
>>    actually populated NUMA, because not every OS will workaround this by
>>    faking the node.
>
>
> According to Andrey Korolyov <andrey@xdel.ru>, memory hotplug should not
> work without SRAT. So maybe forcing to create a NUMA node and SRAT will be
> better idea.
>
> I'm have been working on it.
>
> Thanks.
>
>
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
>>
>> Have you tested this patch with Windows guest?
>>
>> Regards,
>> Hu
>>
>>
>

Thanks, is there will be a place to guard against misconfiguration in
the dimm properties for NUMA too? For example, right now I may specify
just one node for a topo and assign dimms to more than one node, which
will pass argument checks and will result in memory allocation errors
in guest.

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Hu Tao]

On Wed, Sep 17, 2014 at 02:19:05PM +0400, Andrey Korolyov wrote:
> On Wed, Sep 17, 2014 at 2:00 PM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> > Add Andrey Korolyov <andrey@xdel.ru>
> >
> > On 09/17/2014 04:32 PM, Hu Tao wrote:
> >>
> >> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
> >>>
> >>> If we do not configure numa option, memory hotplug should work as well.
> >>> It should not depend on numa option.
> >>>
> >>> Steps to reproduce:
> >>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> >>> (2) Hotplug memory
> >>> It will fail and reports:
> >>> "'DIMM property node has value 0' which exceeds the number of numa nodes:
> >>> 0"
> >>>
> >> I rememberd Tang Chen had a patch for this bug, this is what Andrey
> >> suggested:
> >>
> >>    I thnk that there should be no
> >>    cases when dimm is plugged (and check from patch is fired up) without
> >>    actually populated NUMA, because not every OS will workaround this by
> >>    faking the node.
> >
> >
> > According to Andrey Korolyov <andrey@xdel.ru>, memory hotplug should not
> > work without SRAT. So maybe forcing to create a NUMA node and SRAT will be
> > better idea.
> >
> > I'm have been working on it.
> >
> > Thanks.
> >
> >
> >>
> >> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
> >>
> >> Have you tested this patch with Windows guest?
> >>
> >> Regards,
> >> Hu
> >>
> >>
> >
> 
> Thanks, is there will be a place to guard against misconfiguration in
> the dimm properties for NUMA too? For example, right now I may specify
> just one node for a topo and assign dimms to more than one node, which
> will pass argument checks and will result in memory allocation errors
> in guest.

Yes, there is, in function pc_dimm_realize().

Regards,
Hu

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Igor Mammedov]

On Wed, 17 Sep 2014 16:32:20 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:

> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
> > If we do not configure numa option, memory hotplug should work as well.
> > It should not depend on numa option.
> > 
> > Steps to reproduce:
> > (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> > (2) Hotplug memory
> > It will fail and reports:
> > "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> > 
> 
> I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:
> 
>   I thnk that there should be no
>   cases when dimm is plugged (and check from patch is fired up) without
>   actually populated NUMA, because not every OS will workaround this by
>   faking the node.
This doesn't take in to account that dimm device by itself has nothing to do
with numa (numa is just optional property of its representation in ACPI land
and nothing else).

In case initial memory is converted to dimm devices, qemu can be
started without numa option and it still must work.

So I'm in favor of this path.

>
> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
> 
> Have you tested this patch with Windows guest?
> 
> Regards,
> Hu

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Igor Mammedov]

On Tue, 16 Sep 2014 18:39:15 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> If we do not configure numa option, memory hotplug should work as well.
> It should not depend on numa option.
> 
> Steps to reproduce:
> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> (2) Hotplug memory
> It will fail and reports:
> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/mem/pc-dimm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 5bfc5b7..a800ea7 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -252,7 +252,7 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
>          error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
>          return;
>      }
> -    if (dimm->node >= nb_numa_nodes) {
> +    if ((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) {
>          error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
>                     PRIu32 "' which exceeds the number of numa nodes: %d",
>                     dimm->node, nb_numa_nodes);

Reviewed-By: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Tang Chen]

Hi Igor,

On 09/19/2014 08:26 PM, Igor Mammedov wrote:
> On Wed, 17 Sep 2014 16:32:20 +0800
> Hu Tao <hutao@cn.fujitsu.com> wrote:
>
>> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
>>> If we do not configure numa option, memory hotplug should work as well.
>>> It should not depend on numa option.
>>>
>>> Steps to reproduce:
>>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
>>> (2) Hotplug memory
>>> It will fail and reports:
>>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
>>>
>> I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:
>>
>>    I thnk that there should be no
>>    cases when dimm is plugged (and check from patch is fired up) without
>>    actually populated NUMA, because not every OS will workaround this by
>>    faking the node.
> This doesn't take in to account that dimm device by itself has nothing to do
> with numa (numa is just optional property of its representation in ACPI land
> and nothing else).
>
> In case initial memory is converted to dimm devices, qemu can be
> started without numa option and it still must work.
>
> So I'm in favor of this path.

I just did some tests. Even if I modify qemu code and make hotpluggable 
bit in SRAT 0,
memory hotplug will still work on Linux guest, which means Linux kernel 
doesn't check
SRAT info after system is up when doing memory hotplug.

I did the following modification in hw/i386/acpi-build.c
-    ram_addr_t hotplugabble_address_space_size =
-        object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
-                                NULL);
+    ram_addr_t hotplugabble_address_space_size = 0;

And when the guest is up, no memory should be hotpluggable, I think. But 
I hot-added
memory successfully.

IMHO, I think memory hotplug should based on ACPI on Linux. And SRAT 
tells system
which memory ranges are hotpluggable, and we should follow it. So I 
think Linux kernel
has some problem in this issue.

I'd like to fix it like this:

1. Send patches to make Linux kernel to check SRAT info when doing 
memory hotplug.
     (Now, SRAT is only checked at boot time.)

2. In qemu, when users gave a memory hotplug option without NUMA 
options, we create
     node0 and node1, and make node1 hotpluggable.
     This is because when using MOVABLE_NODE, node0 in which the kernel 
resides in should
     not be hotpluggable.
     Of course, make part of memory in node0 hotpluggable is OK, but on 
a real machine, no
     one will do this, I think. So I suggest above idea.

Thanks. :)

>
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
>>
>> Have you tested this patch with Windows guest?
>>
>> Regards,
>> Hu
>
> .
>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[zhanghailiang]

On 2014/9/22 17:03, Tang Chen wrote:
> Hi Igor,
>
> On 09/19/2014 08:26 PM, Igor Mammedov wrote:
>> On Wed, 17 Sep 2014 16:32:20 +0800
>> Hu Tao <hutao@cn.fujitsu.com> wrote:
>>
>>> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
>>>> If we do not configure numa option, memory hotplug should work as well.
>>>> It should not depend on numa option.
>>>>
>>>> Steps to reproduce:
>>>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
>>>> (2) Hotplug memory
>>>> It will fail and reports:
>>>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
>>>>
>>> I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:
>>>
>>>    I thnk that there should be no
>>>    cases when dimm is plugged (and check from patch is fired up) without
>>>    actually populated NUMA, because not every OS will workaround this by
>>>    faking the node.
>> This doesn't take in to account that dimm device by itself has nothing to do
>> with numa (numa is just optional property of its representation in ACPI land
>> and nothing else).
>>
>> In case initial memory is converted to dimm devices, qemu can be

It seems that if we support pc-dimm memory without numa option in startup,
we still need this patch...

>> started without numa option and it still must work.
>>
>> So I'm in favor of this path.
>
> I just did some tests. Even if I modify qemu code and make hotpluggable bit in SRAT 0,
> memory hotplug will still work on Linux guest, which means Linux kernel doesn't check
> SRAT info after system is up when doing memory hotplug.
>
> I did the following modification in hw/i386/acpi-build.c
> -    ram_addr_t hotplugabble_address_space_size =
> -        object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
> -                                NULL);
> +    ram_addr_t hotplugabble_address_space_size = 0;
>
> And when the guest is up, no memory should be hotpluggable, I think. But I hot-added
> memory successfully.
>
> IMHO, I think memory hotplug should based on ACPI on Linux. And SRAT tells system
> which memory ranges are hotpluggable, and we should follow it. So I think Linux kernel
> has some problem in this issue.
>
> I'd like to fix it like this:
>
> 1. Send patches to make Linux kernel to check SRAT info when doing memory hotplug.
>      (Now, SRAT is only checked at boot time.)
>
> 2. In qemu, when users gave a memory hotplug option without NUMA options, we create
>      node0 and node1, and make node1 hotpluggable.

In this case, guest os will see two numa node even if we don't configure any numa option.

>      This is because when using MOVABLE_NODE, node0 in which the kernel resides in should
>      not be hotpluggable.
>      Of course, make part of memory in node0 hotpluggable is OK, but on a real machine, no
>      one will do this, I think. So I suggest above idea.

In actual usage scenario, usually we use the guest numa cooperating with the host numa,
It may be strange that we can not hotplug memory to guest node0...

>
> Thanks. :)
>
>>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
>>>
>>> Have you tested this patch with Windows guest?
>>>
>>> Regards,
>>> Hu
>>
>> .
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Michael S. Tsirkin]

On Fri, Sep 19, 2014 at 02:37:46PM +0200, Igor Mammedov wrote:
> On Tue, 16 Sep 2014 18:39:15 +0800
> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
> 
> > If we do not configure numa option, memory hotplug should work as well.
> > It should not depend on numa option.
> > 
> > Steps to reproduce:
> > (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> > (2) Hotplug memory
> > It will fail and reports:
> > "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> > 
> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> > ---
> >  hw/mem/pc-dimm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index 5bfc5b7..a800ea7 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -252,7 +252,7 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
> >          error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
> >          return;
> >      }
> > -    if (dimm->node >= nb_numa_nodes) {
> > +    if ((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) {
> >          error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
> >                     PRIu32 "' which exceeds the number of numa nodes: %d",
> >                     dimm->node, nb_numa_nodes);
> 
> Reviewed-By: Igor Mammedov <imammedo@redhat.com>


I read:
> Hmm, I have just tested this, and Yes, it didn't work for Windows guest.
> Thanks for your kind reminder.;)

So should I expect v2 which works with windows?

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Igor Mammedov]

On Mon, 22 Sep 2014 17:03:12 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> Hi Igor,
> 
> On 09/19/2014 08:26 PM, Igor Mammedov wrote:
> > On Wed, 17 Sep 2014 16:32:20 +0800
> > Hu Tao <hutao@cn.fujitsu.com> wrote:
> >
> >> On Tue, Sep 16, 2014 at 06:39:15PM +0800, zhanghailiang wrote:
> >>> If we do not configure numa option, memory hotplug should work as well.
> >>> It should not depend on numa option.
> >>>
> >>> Steps to reproduce:
> >>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> >>> (2) Hotplug memory
> >>> It will fail and reports:
> >>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> >>>
> >> I rememberd Tang Chen had a patch for this bug, this is what Andrey suggested:
> >>
> >>    I thnk that there should be no
> >>    cases when dimm is plugged (and check from patch is fired up) without
> >>    actually populated NUMA, because not every OS will workaround this by
> >>    faking the node.
> > This doesn't take in to account that dimm device by itself has nothing to do
> > with numa (numa is just optional property of its representation in ACPI land
> > and nothing else).
> >
> > In case initial memory is converted to dimm devices, qemu can be
> > started without numa option and it still must work.
> >
> > So I'm in favor of this path.
> 
> I just did some tests. Even if I modify qemu code and make hotpluggable 
> bit in SRAT 0,
> memory hotplug will still work on Linux guest, which means Linux kernel 
> doesn't check
> SRAT info after system is up when doing memory hotplug.
> 
> I did the following modification in hw/i386/acpi-build.c
> -    ram_addr_t hotplugabble_address_space_size =
> -        object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
> -                                NULL);
> +    ram_addr_t hotplugabble_address_space_size = 0;
> 
> And when the guest is up, no memory should be hotpluggable, I think. But 
> I hot-added
> memory successfully.
> 
> IMHO, I think memory hotplug should based on ACPI on Linux. And SRAT 
> tells system
> which memory ranges are hotpluggable, and we should follow it. So I 
> think Linux kernel
> has some problem in this issue.
It's fine to use SRAT for these purposes on baremetal NUMA systems since
due to used chipset constrains it's possible statically allocate ranges
for every possible DIMM socket.
However SRAT(which is optional table BTW) entries are not mandatory
and override-able by ACPI Device's _PXM/_CRS methods replacing needs
for SRAT entries and QEMU uses this fact by supplying these methods.
QEMU adds FAKE SRAT entry only to workaround Windows limitation,
and for nothing else.

I think Linux does not violate ACPI spec and behaves as expected, moreover
it's more correct than Windows since memory hotplug will work on non NUMA
machines as well.

Hence I think this patch is correct and allows memory hotplug in absence
of NUMA configuration. It also would allow to use pc-dimm as replacement
for initial memory for non-NUMA configs (which is on my TODO list)

As for the Windows, QEMU has no idea what OS it would be running,
I see 2 ways to solve issue:
 1. user should know that memory hotplug on Windows requires NUMA machine
    and specify "-numa ..." option for this case.
   (I've discussed this with libvirt folks and was promised that
    if user enables memory hotplug, libvirt would provide "-numa" option
    to workaround Windows issue)

 2. QEMU could unconditionally create single NUMA if memory hotplug is
    enabled. (but that should be enable only for 2.2 or late machines
    to avoid migration issues)

> 
> I'd like to fix it like this:
> 
> 1. Send patches to make Linux kernel to check SRAT info when doing 
> memory hotplug.
>      (Now, SRAT is only checked at boot time.)
> 
> 2. In qemu, when users gave a memory hotplug option without NUMA 
> options, we create
>      node0 and node1, and make node1 hotpluggable.
>      This is because when using MOVABLE_NODE, node0 in which the kernel 
> resides in should
>      not be hotpluggable.
>      Of course, make part of memory in node0 hotpluggable is OK, but on 
> a real machine, no
>      one will do this, I think. So I suggest above idea.
> 
> Thanks. :)
> 
> >
> >> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04587.html
> >>
> >> Have you tested this patch with Windows guest?
> >>
> >> Regards,
> >> Hu
> >
> > .
> >
> 
> 

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Tang Chen]

On 09/23/2014 04:40 PM, Igor Mammedov wrote:
> ......
> It's fine to use SRAT for these purposes on baremetal NUMA systems since
> due to used chipset constrains it's possible statically allocate ranges
> for every possible DIMM socket.
> However SRAT(which is optional table BTW) entries are not mandatory
> and override-able by ACPI Device's _PXM/_CRS methods replacing needs
> for SRAT entries and QEMU uses this fact by supplying these methods.
> QEMU adds FAKE SRAT entry only to workaround Windows limitation,
> and for nothing else.
>
> I think Linux does not violate ACPI spec and behaves as expected, moreover
> it's more correct than Windows since memory hotplug will work on non NUMA
> machines as well.
>
> Hence I think this patch is correct and allows memory hotplug in absence
> of NUMA configuration. It also would allow to use pc-dimm as replacement
> for initial memory for non-NUMA configs (which is on my TODO list)
>
> As for the Windows, QEMU has no idea what OS it would be running,
> I see 2 ways to solve issue:
>   1. user should know that memory hotplug on Windows requires NUMA machine
>      and specify "-numa ..." option for this case.
>     (I've discussed this with libvirt folks and was promised that
>      if user enables memory hotplug, libvirt would provide "-numa" option
>      to workaround Windows issue)
>
>   2. QEMU could unconditionally create single NUMA if memory hotplug is
>      enabled. (but that should be enable only for 2.2 or late machines
>      to avoid migration issues)
>
I prefer 2. I'll try to send patches for it if Zhang is also OK with it.

Thanks. :)

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Igor Mammedov]

On Mon, 22 Sep 2014 14:17:28 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Sep 19, 2014 at 02:37:46PM +0200, Igor Mammedov wrote:
> > On Tue, 16 Sep 2014 18:39:15 +0800
> > zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
> > 
> > > If we do not configure numa option, memory hotplug should work as well.
> > > It should not depend on numa option.
> > > 
> > > Steps to reproduce:
> > > (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> > > (2) Hotplug memory
> > > It will fail and reports:
> > > "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> > > 
> > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> > > ---
> > >  hw/mem/pc-dimm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index 5bfc5b7..a800ea7 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -252,7 +252,7 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
> > >          error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
> > >          return;
> > >      }
> > > -    if (dimm->node >= nb_numa_nodes) {
> > > +    if ((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) {
> > >          error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
> > >                     PRIu32 "' which exceeds the number of numa nodes: %d",
> > >                     dimm->node, nb_numa_nodes);
> > 
> > Reviewed-By: Igor Mammedov <imammedo@redhat.com>
> 
> 
> I read:
> > Hmm, I have just tested this, and Yes, it didn't work for Windows guest.
> > Thanks for your kind reminder.;)
> 
> So should I expect v2 which works with windows?
Hotplug wouldn't work with Windows without -numa (it's Windows limitation)
and more importantly pc-dimm shouldn't be limited only to NUMA configs
which this patch fixes.
This patch is fine and should go to stable as well.

On top of this we could add automatic NUMA node creation when
memory hotplug is enabled if this Windows workaround is acceptable.

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[zhanghailiang]

On 2014/9/23 17:01, Igor Mammedov wrote:
> On Mon, 22 Sep 2014 14:17:28 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Fri, Sep 19, 2014 at 02:37:46PM +0200, Igor Mammedov wrote:
>>> On Tue, 16 Sep 2014 18:39:15 +0800
>>> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
>>>
>>>> If we do not configure numa option, memory hotplug should work as well.
>>>> It should not depend on numa option.
>>>>
>>>> Steps to reproduce:
>>>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
>>>> (2) Hotplug memory
>>>> It will fail and reports:
>>>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>   hw/mem/pc-dimm.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>>>> index 5bfc5b7..a800ea7 100644
>>>> --- a/hw/mem/pc-dimm.c
>>>> +++ b/hw/mem/pc-dimm.c
>>>> @@ -252,7 +252,7 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
>>>>           error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
>>>>           return;
>>>>       }
>>>> -    if (dimm->node >= nb_numa_nodes) {
>>>> +    if ((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) {
>>>>           error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
>>>>                      PRIu32 "' which exceeds the number of numa nodes: %d",
>>>>                      dimm->node, nb_numa_nodes);
>>>
>>> Reviewed-By: Igor Mammedov <imammedo@redhat.com>
>>
>>
>> I read:
>>> Hmm, I have just tested this, and Yes, it didn't work for Windows guest.
>>> Thanks for your kind reminder.;)
>>
>> So should I expect v2 which works with windows?
> Hotplug wouldn't work with Windows without -numa (it's Windows limitation)
> and more importantly pc-dimm shouldn't be limited only to NUMA configs
> which this patch fixes.
> This patch is fine and should go to stable as well.
>

Agreed, maybe i should change the title and submit V2.;)

> On top of this we could add automatic NUMA node creation when
> memory hotplug is enabled if this Windows workaround is acceptable.
>
>
> .
>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[zhanghailiang]

On 2014/9/23 16:58, Tang Chen wrote:
>
> On 09/23/2014 04:40 PM, Igor Mammedov wrote:
>> ......
>> It's fine to use SRAT for these purposes on baremetal NUMA systems since
>> due to used chipset constrains it's possible statically allocate ranges
>> for every possible DIMM socket.
>> However SRAT(which is optional table BTW) entries are not mandatory
>> and override-able by ACPI Device's _PXM/_CRS methods replacing needs
>> for SRAT entries and QEMU uses this fact by supplying these methods.
>> QEMU adds FAKE SRAT entry only to workaround Windows limitation,
>> and for nothing else.
>>
>> I think Linux does not violate ACPI spec and behaves as expected, moreover
>> it's more correct than Windows since memory hotplug will work on non NUMA
>> machines as well.
>>
>> Hence I think this patch is correct and allows memory hotplug in absence
>> of NUMA configuration. It also would allow to use pc-dimm as replacement
>> for initial memory for non-NUMA configs (which is on my TODO list)
>>
>> As for the Windows, QEMU has no idea what OS it would be running,
>> I see 2 ways to solve issue:
>>   1. user should know that memory hotplug on Windows requires NUMA machine
>>      and specify "-numa ..." option for this case.
>>     (I've discussed this with libvirt folks and was promised that
>>      if user enables memory hotplug, libvirt would provide "-numa" option
>>      to workaround Windows issue)
>>
>>   2. QEMU could unconditionally create single NUMA if memory hotplug is
>>      enabled. (but that should be enable only for 2.2 or late machines
>>      to avoid migration issues)
>>
> I prefer 2. I'll try to send patches for it if Zhang is also OK with it.
>

Yep, It is a good scheme to create a dummy NUMA unconditionally.
But Igor has said there are migration issues for this scenario, do you know what's
it? ;)

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Tang Chen]

On 09/23/2014 06:11 PM, zhanghailiang wrote:
> On 2014/9/23 16:58, Tang Chen wrote:
>>
>> On 09/23/2014 04:40 PM, Igor Mammedov wrote:
>>> ......
>>> It's fine to use SRAT for these purposes on baremetal NUMA systems 
>>> since
>>> due to used chipset constrains it's possible statically allocate ranges
>>> for every possible DIMM socket.
>>> However SRAT(which is optional table BTW) entries are not mandatory
>>> and override-able by ACPI Device's _PXM/_CRS methods replacing needs
>>> for SRAT entries and QEMU uses this fact by supplying these methods.
>>> QEMU adds FAKE SRAT entry only to workaround Windows limitation,
>>> and for nothing else.
>>>
>>> I think Linux does not violate ACPI spec and behaves as expected, 
>>> moreover
>>> it's more correct than Windows since memory hotplug will work on non 
>>> NUMA
>>> machines as well.
>>>
>>> Hence I think this patch is correct and allows memory hotplug in 
>>> absence
>>> of NUMA configuration. It also would allow to use pc-dimm as 
>>> replacement
>>> for initial memory for non-NUMA configs (which is on my TODO list)
>>>
>>> As for the Windows, QEMU has no idea what OS it would be running,
>>> I see 2 ways to solve issue:
>>>   1. user should know that memory hotplug on Windows requires NUMA 
>>> machine
>>>      and specify "-numa ..." option for this case.
>>>     (I've discussed this with libvirt folks and was promised that
>>>      if user enables memory hotplug, libvirt would provide "-numa" 
>>> option
>>>      to workaround Windows issue)
>>>
>>>   2. QEMU could unconditionally create single NUMA if memory hotplug is
>>>      enabled. (but that should be enable only for 2.2 or late machines
>>>      to avoid migration issues)
>>>
>> I prefer 2. I'll try to send patches for it if Zhang is also OK with it.
>>
>
> Yep, It is a good scheme to create a dummy NUMA unconditionally.
> But Igor has said there are migration issues for this scenario, do you 
> know what's
> it? ;)

Not sure.  This one ?

https://www.mail-archive.com/qemu-devel%40nongnu.org/msg249146.html

Thanks. :)

>
> .
>

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Igor Mammedov]

On Tue, 23 Sep 2014 18:11:35 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> On 2014/9/23 16:58, Tang Chen wrote:
> >
> > On 09/23/2014 04:40 PM, Igor Mammedov wrote:
> >> ......
> >> It's fine to use SRAT for these purposes on baremetal NUMA systems since
> >> due to used chipset constrains it's possible statically allocate ranges
> >> for every possible DIMM socket.
> >> However SRAT(which is optional table BTW) entries are not mandatory
> >> and override-able by ACPI Device's _PXM/_CRS methods replacing needs
> >> for SRAT entries and QEMU uses this fact by supplying these methods.
> >> QEMU adds FAKE SRAT entry only to workaround Windows limitation,
> >> and for nothing else.
> >>
> >> I think Linux does not violate ACPI spec and behaves as expected, moreover
> >> it's more correct than Windows since memory hotplug will work on non NUMA
> >> machines as well.
> >>
> >> Hence I think this patch is correct and allows memory hotplug in absence
> >> of NUMA configuration. It also would allow to use pc-dimm as replacement
> >> for initial memory for non-NUMA configs (which is on my TODO list)
> >>
> >> As for the Windows, QEMU has no idea what OS it would be running,
> >> I see 2 ways to solve issue:
> >>   1. user should know that memory hotplug on Windows requires NUMA machine
> >>      and specify "-numa ..." option for this case.
> >>     (I've discussed this with libvirt folks and was promised that
> >>      if user enables memory hotplug, libvirt would provide "-numa" option
> >>      to workaround Windows issue)
> >>
> >>   2. QEMU could unconditionally create single NUMA if memory hotplug is
> >>      enabled. (but that should be enable only for 2.2 or late machines
> >>      to avoid migration issues)
> >>
> > I prefer 2. I'll try to send patches for it if Zhang is also OK with it.
> >
> 
> Yep, It is a good scheme to create a dummy NUMA unconditionally.
> But Igor has said there are migration issues for this scenario, do you know what's
> it? ;)
> 
'-numa' will add SRAT table to ACPI tables blob, as result it will grow in size,
depending on config options #cpus, #dimms, #pci-bridges it could trigger
issues we've had with prior 2.1 was released.

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[Igor Mammedov]

On Tue, 23 Sep 2014 18:07:16 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> On 2014/9/23 17:01, Igor Mammedov wrote:
> > On Mon, 22 Sep 2014 14:17:28 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Fri, Sep 19, 2014 at 02:37:46PM +0200, Igor Mammedov wrote:
> >>> On Tue, 16 Sep 2014 18:39:15 +0800
> >>> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
> >>>
> >>>> If we do not configure numa option, memory hotplug should work as well.
> >>>> It should not depend on numa option.
> >>>>
> >>>> Steps to reproduce:
> >>>> (1) Start VM: qemu-kvm -m 1024,slots=4,maxmem=8G
> >>>> (2) Hotplug memory
> >>>> It will fail and reports:
> >>>> "'DIMM property node has value 0' which exceeds the number of numa nodes: 0"
> >>>>
> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> >>>> ---
> >>>>   hw/mem/pc-dimm.c | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >>>> index 5bfc5b7..a800ea7 100644
> >>>> --- a/hw/mem/pc-dimm.c
> >>>> +++ b/hw/mem/pc-dimm.c
> >>>> @@ -252,7 +252,7 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
> >>>>           error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
> >>>>           return;
> >>>>       }
> >>>> -    if (dimm->node >= nb_numa_nodes) {
> >>>> +    if ((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) {
> >>>>           error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
> >>>>                      PRIu32 "' which exceeds the number of numa nodes: %d",
> >>>>                      dimm->node, nb_numa_nodes);
> >>>
> >>> Reviewed-By: Igor Mammedov <imammedo@redhat.com>
> >>
> >>
> >> I read:
> >>> Hmm, I have just tested this, and Yes, it didn't work for Windows guest.
> >>> Thanks for your kind reminder.;)
> >>
> >> So should I expect v2 which works with windows?
> > Hotplug wouldn't work with Windows without -numa (it's Windows limitation)
> > and more importantly pc-dimm shouldn't be limited only to NUMA configs
> > which this patch fixes.
> > This patch is fine and should go to stable as well.
> >
> 
> Agreed, maybe i should change the title and submit V2.;)
sure

> 
> > On top of this we could add automatic NUMA node creation when
> > memory hotplug is enabled if this Windows workaround is acceptable.
> >
> >
> > .
> >
> 
> 

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: No numa option shouldn't break hotplug memory feature

[zhanghailiang]

On 2014/9/23 19:12, Igor Mammedov wrote:
> On Tue, 23 Sep 2014 18:11:35 +0800
> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
>
>> On 2014/9/23 16:58, Tang Chen wrote:
>>>
>>> On 09/23/2014 04:40 PM, Igor Mammedov wrote:
>>>> ......
>>>> It's fine to use SRAT for these purposes on baremetal NUMA systems since
>>>> due to used chipset constrains it's possible statically allocate ranges
>>>> for every possible DIMM socket.
>>>> However SRAT(which is optional table BTW) entries are not mandatory
>>>> and override-able by ACPI Device's _PXM/_CRS methods replacing needs
>>>> for SRAT entries and QEMU uses this fact by supplying these methods.
>>>> QEMU adds FAKE SRAT entry only to workaround Windows limitation,
>>>> and for nothing else.
>>>>
>>>> I think Linux does not violate ACPI spec and behaves as expected, moreover
>>>> it's more correct than Windows since memory hotplug will work on non NUMA
>>>> machines as well.
>>>>
>>>> Hence I think this patch is correct and allows memory hotplug in absence
>>>> of NUMA configuration. It also would allow to use pc-dimm as replacement
>>>> for initial memory for non-NUMA configs (which is on my TODO list)
>>>>
>>>> As for the Windows, QEMU has no idea what OS it would be running,
>>>> I see 2 ways to solve issue:
>>>>    1. user should know that memory hotplug on Windows requires NUMA machine
>>>>       and specify "-numa ..." option for this case.
>>>>      (I've discussed this with libvirt folks and was promised that
>>>>       if user enables memory hotplug, libvirt would provide "-numa" option
>>>>       to workaround Windows issue)
>>>>
>>>>    2. QEMU could unconditionally create single NUMA if memory hotplug is
>>>>       enabled. (but that should be enable only for 2.2 or late machines
>>>>       to avoid migration issues)
>>>>
>>> I prefer 2. I'll try to send patches for it if Zhang is also OK with it.
>>>
>>
>> Yep, It is a good scheme to create a dummy NUMA unconditionally.
>> But Igor has said there are migration issues for this scenario, do you know what's
>> it? ;)
>>
> '-numa' will add SRAT table to ACPI tables blob, as result it will grow in size,
> depending on config options #cpus, #dimms, #pci-bridges it could trigger
> issues we've had with prior 2.1 was released.

Hmm, i guess i know what happened, i have found in function acpi_build
there are annotations for the migration issue...
I will look deep into it, Thanks for your patient explanation.:)