[Qemu-devel] [PATCH 0/2] pc: memory hotplug fixes

[Qemu-devel] [PATCH 0/2] pc: memory hotplug fixes

[Igor Mammedov]

Series is build on mst/pci tree that includes memory hotplug bits.

patch 2/2, fixes incorrect address auto-allocation caused by wrong
sorting order due to overflow in pc_dimm_addr_sort() comparator.

Test-case is to build qemu with stderr trace backend and start qemu with
following command line:

echo mhp_pc_dimm_assigned_address > /tmp/mhp
qemu-system-x86_64 -m 512,slots=62,maxmem=16G \
 -object memory-backend-ram,id=m2,size=256M -object memory-backend-ram,id=m3,size=256M \
 -object memory-backend-ram,id=m4,size=256M -object memory-backend-ram,id=m5,size=256M \
 -object memory-backend-ram,id=m6,size=256M -object memory-backend-ram,id=m7,size=256M \
 -object memory-backend-ram,id=m8,size=256M -object memory-backend-ram,id=m9,size=256M \
 -object memory-backend-ram,id=m10,size=256M -object memory-backend-ram,id=m11,size=256M \
 -object memory-backend-ram,id=m12,size=256M -object memory-backend-ram,id=m13,size=256M \
 -device pc-dimm,id=d1,memdev=m1 -device pc-dimm,id=d2,memdev=m2 -device pc-dimm,id=d3,memdev=m3 \
 -device pc-dimm,id=d4,memdev=m4 -device pc-dimm,id=d5,memdev=m5 -device pc-dimm,id=d6,memdev=m6 \
 -device pc-dimm,id=d7,memdev=m7 -device pc-dimm,id=d8,memdev=m8 -device pc-dimm,id=d9,memdev=m9 \
 -device pc-dimm,id=d10,memdev=m10 -device pc-dimm,id=d11,memdev=m11 -device pc-dimm,id=d12,memdev=m12 \
 -device pc-dimm,id=d13,memdev=m13 \
 -trace events=/tmp/mhp

output with incorrect addesses would look like:
mhp_pc_dimm_assigned_address 0x100000000
mhp_pc_dimm_assigned_address 0x110000000
mhp_pc_dimm_assigned_address 0x120000000
mhp_pc_dimm_assigned_address 0x130000000
mhp_pc_dimm_assigned_address 0x140000000
mhp_pc_dimm_assigned_address 0x150000000
mhp_pc_dimm_assigned_address 0x160000000
mhp_pc_dimm_assigned_address 0x170000000
mhp_pc_dimm_assigned_address 0x180000000
mhp_pc_dimm_assigned_address 0x180000000
mhp_pc_dimm_assigned_address 0x180000000
mhp_pc_dimm_assigned_address 0x180000000
mhp_pc_dimm_assigned_address 0x180000000

where last 5 dimms were assigned the same address.

Igor Mammedov (2):
  pc: q35: acpi: report error to user on unsupported unplug request
  pc-dimm: fix large address sorting during auto-allocation

 hw/isa/lpc_ich9.c | 2 ++
 hw/mem/pc-dimm.c  | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] pc: q35: acpi: report error to user on unsupported unplug request

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/isa/lpc_ich9.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 2adf29a..01ca071 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -610,6 +610,8 @@ static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
 static void ich9_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp)
 {
+    error_setg(errp, "acpi: device unplug request for not supported device"
+               " type: %s", object_get_typename(OBJECT(dev)));
 }
 
 static bool ich9_rst_cnt_needed(void *opaque)
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] pc-dimm: fix large address sorting during auto-allocation

[Igor Mammedov]

GCompareFunc(a, b) used by g_slist_insert_sorted() return 'gint',
however it might be too small to fit difference between
2 addresses. So use 128bit calculate difference and normalize
result to -1/0/1 return values.

Reported-By: Andrey Korolyov <andrey@xdel.ru>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/mem/pc-dimm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9f091c6..8c26568 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -73,8 +73,14 @@ static gint pc_dimm_addr_sort(gconstpointer a, gconstpointer b)
 {
     PCDIMMDevice *x = PC_DIMM(a);
     PCDIMMDevice *y = PC_DIMM(b);
+    Int128 diff = int128_sub(int128_make64(x->addr), int128_make64(y->addr));
 
-    return x->addr - y->addr;
+    if (int128_lt(diff, int128_zero())) {
+        return -1;
+    } else if (int128_gt(diff, int128_zero())) {
+        return 1;
+    }
+    return 0;
 }
 
 static int pc_dimm_built_list(Object *obj, void *opaque)
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/2] pc: memory hotplug fixes

[Michael S. Tsirkin]

On Mon, Jun 09, 2014 at 07:27:59PM +0200, Igor Mammedov wrote:
> Series is build on mst/pci tree that includes memory hotplug bits.
> 
> patch 2/2, fixes incorrect address auto-allocation caused by wrong
> sorting order due to overflow in pc_dimm_addr_sort() comparator.
> 
> Test-case is to build qemu with stderr trace backend and start qemu with
> following command line:
> 
> echo mhp_pc_dimm_assigned_address > /tmp/mhp
> qemu-system-x86_64 -m 512,slots=62,maxmem=16G \
>  -object memory-backend-ram,id=m2,size=256M -object memory-backend-ram,id=m3,size=256M \
>  -object memory-backend-ram,id=m4,size=256M -object memory-backend-ram,id=m5,size=256M \
>  -object memory-backend-ram,id=m6,size=256M -object memory-backend-ram,id=m7,size=256M \
>  -object memory-backend-ram,id=m8,size=256M -object memory-backend-ram,id=m9,size=256M \
>  -object memory-backend-ram,id=m10,size=256M -object memory-backend-ram,id=m11,size=256M \
>  -object memory-backend-ram,id=m12,size=256M -object memory-backend-ram,id=m13,size=256M \
>  -device pc-dimm,id=d1,memdev=m1 -device pc-dimm,id=d2,memdev=m2 -device pc-dimm,id=d3,memdev=m3 \
>  -device pc-dimm,id=d4,memdev=m4 -device pc-dimm,id=d5,memdev=m5 -device pc-dimm,id=d6,memdev=m6 \
>  -device pc-dimm,id=d7,memdev=m7 -device pc-dimm,id=d8,memdev=m8 -device pc-dimm,id=d9,memdev=m9 \
>  -device pc-dimm,id=d10,memdev=m10 -device pc-dimm,id=d11,memdev=m11 -device pc-dimm,id=d12,memdev=m12 \
>  -device pc-dimm,id=d13,memdev=m13 \
>  -trace events=/tmp/mhp
> 
> output with incorrect addesses would look like:
> mhp_pc_dimm_assigned_address 0x100000000
> mhp_pc_dimm_assigned_address 0x110000000
> mhp_pc_dimm_assigned_address 0x120000000
> mhp_pc_dimm_assigned_address 0x130000000
> mhp_pc_dimm_assigned_address 0x140000000
> mhp_pc_dimm_assigned_address 0x150000000
> mhp_pc_dimm_assigned_address 0x160000000
> mhp_pc_dimm_assigned_address 0x170000000
> mhp_pc_dimm_assigned_address 0x180000000
> mhp_pc_dimm_assigned_address 0x180000000
> mhp_pc_dimm_assigned_address 0x180000000
> mhp_pc_dimm_assigned_address 0x180000000
> mhp_pc_dimm_assigned_address 0x180000000
> 
> where last 5 dimms were assigned the same address.

I rebased my tree and applied these.
Pushed, pls check the result.

> Igor Mammedov (2):
>   pc: q35: acpi: report error to user on unsupported unplug request
>   pc-dimm: fix large address sorting during auto-allocation
> 
>  hw/isa/lpc_ich9.c | 2 ++
>  hw/mem/pc-dimm.c  | 8 +++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH 0/2] pc: memory hotplug fixes

[Igor Mammedov]

On Tue, 10 Jun 2014 14:05:07 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jun 09, 2014 at 07:27:59PM +0200, Igor Mammedov wrote:
> > Series is build on mst/pci tree that includes memory hotplug bits.
> > 
> > patch 2/2, fixes incorrect address auto-allocation caused by wrong
> > sorting order due to overflow in pc_dimm_addr_sort() comparator.
> > 
> > Test-case is to build qemu with stderr trace backend and start qemu with
> > following command line:
> > 
> > echo mhp_pc_dimm_assigned_address > /tmp/mhp
> > qemu-system-x86_64 -m 512,slots=62,maxmem=16G \
> >  -object memory-backend-ram,id=m2,size=256M -object memory-backend-ram,id=m3,size=256M \
> >  -object memory-backend-ram,id=m4,size=256M -object memory-backend-ram,id=m5,size=256M \
> >  -object memory-backend-ram,id=m6,size=256M -object memory-backend-ram,id=m7,size=256M \
> >  -object memory-backend-ram,id=m8,size=256M -object memory-backend-ram,id=m9,size=256M \
> >  -object memory-backend-ram,id=m10,size=256M -object memory-backend-ram,id=m11,size=256M \
> >  -object memory-backend-ram,id=m12,size=256M -object memory-backend-ram,id=m13,size=256M \
> >  -device pc-dimm,id=d1,memdev=m1 -device pc-dimm,id=d2,memdev=m2 -device pc-dimm,id=d3,memdev=m3 \
> >  -device pc-dimm,id=d4,memdev=m4 -device pc-dimm,id=d5,memdev=m5 -device pc-dimm,id=d6,memdev=m6 \
> >  -device pc-dimm,id=d7,memdev=m7 -device pc-dimm,id=d8,memdev=m8 -device pc-dimm,id=d9,memdev=m9 \
> >  -device pc-dimm,id=d10,memdev=m10 -device pc-dimm,id=d11,memdev=m11 -device pc-dimm,id=d12,memdev=m12 \
> >  -device pc-dimm,id=d13,memdev=m13 \
> >  -trace events=/tmp/mhp
> > 
> > output with incorrect addesses would look like:
> > mhp_pc_dimm_assigned_address 0x100000000
> > mhp_pc_dimm_assigned_address 0x110000000
> > mhp_pc_dimm_assigned_address 0x120000000
> > mhp_pc_dimm_assigned_address 0x130000000
> > mhp_pc_dimm_assigned_address 0x140000000
> > mhp_pc_dimm_assigned_address 0x150000000
> > mhp_pc_dimm_assigned_address 0x160000000
> > mhp_pc_dimm_assigned_address 0x170000000
> > mhp_pc_dimm_assigned_address 0x180000000
> > mhp_pc_dimm_assigned_address 0x180000000
> > mhp_pc_dimm_assigned_address 0x180000000
> > mhp_pc_dimm_assigned_address 0x180000000
> > mhp_pc_dimm_assigned_address 0x180000000
> > 
> > where last 5 dimms were assigned the same address.
> 
> I rebased my tree and applied these.
> Pushed, pls check the result.
result looks fine
thanks

> > Igor Mammedov (2):
> >   pc: q35: acpi: report error to user on unsupported unplug request
> >   pc-dimm: fix large address sorting during auto-allocation
> > 
> >  hw/isa/lpc_ich9.c | 2 ++
> >  hw/mem/pc-dimm.c  | 8 +++++++-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > -- 
> > 1.9.3


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 0/2] pc: memory hotplug fixes

[Vasilis Liaskovitis]

On Mon, Jun 09, 2014 at 07:27:59PM +0200, Igor Mammedov wrote:
> Series is build on mst/pci tree that includes memory hotplug bits.
> 
> patch 2/2, fixes incorrect address auto-allocation caused by wrong
> sorting order due to overflow in pc_dimm_addr_sort() comparator.
>

thanks for the fixes. 
For future hotplug related fixes, please cc Mohammed Gamal and Anshul Makkar
(I added them in cc above) instead of me.

thanks,

- Vasilis