[Qemu-devel] [PATCH v3 0/2] pc: memhp: enforce gaps between DIMMs

[Qemu-devel] [PATCH v3 0/2] pc: memhp: enforce gaps between DIMMs

[Igor Mammedov]

v3:
 -  use pc_machine_class_init() for default. (Eduardo)
 -  make 'gap' boolean and just add 1 byte gap
    when calculating new_addr for DIMM.
v2:
   make inter_dimm_gap a boolean and inster gap in 1 byte
   instead of 2Mb, due to alignment that gap would be extended
   up to natural backend alignment value.

it's a simplier way suggested by Michael S. Tsirkin
to workaround virtio bug reported earlier:
http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
where virtio can't handle buffer that crosses border
between 2 DIMM's (i.e. 2 MemoryRegions).

idea is to leave gaps between DIMMs, making their GPAs
non contiguous, which effectively forces kmalloc
to not use DIMM if buffer doesn't fit inside of it.

Simpler reproducer:
 qemu-system-x86_64 -enable-kvm  -m 256,slots=250,maxmem=32G  -drive if=virtio,file=/dev/slow/rhel72  \
  `for i in $(seq 0 58); do echo -n "-object memory-backend-ram,id=m$i,size=10M -device pc-dimm,id=dimm$i,memdev=m$i "; done` \
   -nodefaults -snapshot -serial stdio -nographic -M pc-i440fx-2.4

if guest manages to boot just login and do:
  dd if=/dev/vda of=/dev/null bs=128M

it will crash QEMU in 99% cases with following message:
  qemu-system-x86_64: virtio: error trying to map MMIO memory

Tested vs master and mst/PCI trees.

Igor Mammedov (2):
  memhp: extend address auto assignment to support gaps
  pc: memhp: force gaps between DIMM's GPA

 hw/i386/pc.c             |  5 ++++-
 hw/i386/pc_piix.c        |  1 +
 hw/i386/pc_q35.c         |  1 +
 hw/mem/pc-dimm.c         | 15 +++++++++------
 hw/ppc/spapr.c           |  2 +-
 include/hw/i386/pc.h     |  1 +
 include/hw/mem/pc-dimm.h |  7 ++++---
 7 files changed, 21 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to support gaps

[Igor Mammedov]

setting gap to TRUE will make sparse DIMM
address auto allocation, leaving gaps between
a new DIMM address and preceeding existing DIMM.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * make 'gap' boolean and just add 1 byte gap
    when calculating new_addr for DIMM.
---
 hw/i386/pc.c             |  3 ++-
 hw/mem/pc-dimm.c         | 15 +++++++++------
 hw/ppc/spapr.c           |  2 +-
 include/hw/mem/pc-dimm.h |  7 ++++---
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 461c128..ef02736 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1644,7 +1644,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
+    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, false,
+                        &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index bb04862..6cc6ac3 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -32,7 +32,8 @@ typedef struct pc_dimms_capacity {
 } pc_dimms_capacity;
 
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
-                         MemoryRegion *mr, uint64_t align, Error **errp)
+                         MemoryRegion *mr, uint64_t align, bool gap,
+                         Error **errp)
 {
     int slot;
     MachineState *machine = MACHINE(qdev_get_machine());
@@ -48,7 +49,7 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
 
     addr = pc_dimm_get_free_addr(hpms->base,
                                  memory_region_size(&hpms->mr),
-                                 !addr ? NULL : &addr, align,
+                                 !addr ? NULL : &addr, align, gap,
                                  memory_region_size(mr), &local_err);
     if (local_err) {
         goto out;
@@ -287,8 +288,8 @@ static int pc_dimm_built_list(Object *obj, void *opaque)
 
 uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
                                uint64_t address_space_size,
-                               uint64_t *hint, uint64_t align, uint64_t size,
-                               Error **errp)
+                               uint64_t *hint, uint64_t align, bool gap,
+                               uint64_t size, Error **errp)
 {
     GSList *list = NULL, *item;
     uint64_t new_addr, ret = 0;
@@ -333,13 +334,15 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
             goto out;
         }
 
-        if (ranges_overlap(dimm->addr, dimm_size, new_addr, size)) {
+        if (ranges_overlap(dimm->addr, dimm_size, new_addr,
+                           size + (gap ? 1 : 0))) {
             if (hint) {
                 DeviceState *d = DEVICE(dimm);
                 error_setg(errp, "address range conflicts with '%s'", d->id);
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size, align);
+            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size + (gap ? 1 : 0),
+                                     align);
         }
     }
     ret = new_addr;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a9b5f2a..d1b0e53 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2096,7 +2096,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
+    pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, false, &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index d83bf30..c1ee7b0 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -83,15 +83,16 @@ typedef struct MemoryHotplugState {
 
 uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
                                uint64_t address_space_size,
-                               uint64_t *hint, uint64_t align, uint64_t size,
-                               Error **errp);
+                               uint64_t *hint, uint64_t align, bool gap,
+                               uint64_t size, Error **errp);
 
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
 uint64_t pc_existing_dimms_capacity(Error **errp);
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
-                         MemoryRegion *mr, uint64_t align, Error **errp);
+                         MemoryRegion *mr, uint64_t align, bool gap,
+                         Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
                            MemoryRegion *mr);
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to support gaps

[Eduardo Habkost]

On Tue, Sep 29, 2015 at 04:53:28PM +0200, Igor Mammedov wrote:
> setting gap to TRUE will make sparse DIMM
> address auto allocation, leaving gaps between
> a new DIMM address and preceeding existing DIMM.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to support gaps

[Bharata B Rao]

On Tue, Sep 29, 2015 at 04:53:28PM +0200, Igor Mammedov wrote:
> setting gap to TRUE will make sparse DIMM
> address auto allocation, leaving gaps between
> a new DIMM address and preceeding existing DIMM.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * make 'gap' boolean and just add 1 byte gap
>     when calculating new_addr for DIMM.
> ---
>  hw/i386/pc.c             |  3 ++-
>  hw/mem/pc-dimm.c         | 15 +++++++++------
>  hw/ppc/spapr.c           |  2 +-
>  include/hw/mem/pc-dimm.h |  7 ++++---
>  4 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 461c128..ef02736 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1644,7 +1644,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
> 
> -    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
> +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, false,
> +                        &local_err);
>      if (local_err) {
>          goto out;
>      }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index bb04862..6cc6ac3 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -32,7 +32,8 @@ typedef struct pc_dimms_capacity {
>  } pc_dimms_capacity;
> 
>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> -                         MemoryRegion *mr, uint64_t align, Error **errp)
> +                         MemoryRegion *mr, uint64_t align, bool gap,
> +                         Error **errp)
>  {
>      int slot;
>      MachineState *machine = MACHINE(qdev_get_machine());
> @@ -48,7 +49,7 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> 
>      addr = pc_dimm_get_free_addr(hpms->base,
>                                   memory_region_size(&hpms->mr),
> -                                 !addr ? NULL : &addr, align,
> +                                 !addr ? NULL : &addr, align, gap,
>                                   memory_region_size(mr), &local_err);
>      if (local_err) {
>          goto out;
> @@ -287,8 +288,8 @@ static int pc_dimm_built_list(Object *obj, void *opaque)
> 
>  uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
>                                 uint64_t address_space_size,
> -                               uint64_t *hint, uint64_t align, uint64_t size,
> -                               Error **errp)
> +                               uint64_t *hint, uint64_t align, bool gap,
> +                               uint64_t size, Error **errp)
>  {
>      GSList *list = NULL, *item;
>      uint64_t new_addr, ret = 0;
> @@ -333,13 +334,15 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
>              goto out;
>          }
> 
> -        if (ranges_overlap(dimm->addr, dimm_size, new_addr, size)) {
> +        if (ranges_overlap(dimm->addr, dimm_size, new_addr,
> +                           size + (gap ? 1 : 0))) {
>              if (hint) {
>                  DeviceState *d = DEVICE(dimm);
>                  error_setg(errp, "address range conflicts with '%s'", d->id);
>                  goto out;
>              }
> -            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size, align);
> +            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size + (gap ? 1 : 0),
> +                                     align);

This change of adding 1 byte gap will break PowerPC memory hotplug in
its current form.

Currently we divide hotpluggable memory region into chuncks of 256M
and there is a DR connector object for each such chunk. The DR connector
object maintains/controls the state transitions of that memory chunk
as per PAPR specifications.

Now after this 1 byte gap, we end up having a 64K alignment (default
page size) for the DIMM address and end up having an address for which
there is no DR connector object. I will have to revisit the parts of
the code in PowerPC that creates DR connector objects and looks them up
by address.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to support gaps

[Bharata B Rao]

On Thu, Oct 08, 2015 at 07:01:09PM +0530, Bharata B Rao wrote:
> > @@ -333,13 +334,15 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
> >              goto out;
> >          }
> > 
> > -        if (ranges_overlap(dimm->addr, dimm_size, new_addr, size)) {
> > +        if (ranges_overlap(dimm->addr, dimm_size, new_addr,
> > +                           size + (gap ? 1 : 0))) {
> >              if (hint) {
> >                  DeviceState *d = DEVICE(dimm);
> >                  error_setg(errp, "address range conflicts with '%s'", d->id);
> >                  goto out;
> >              }
> > -            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size, align);
> > +            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size + (gap ? 1 : 0),
> > +                                     align);
> 
> This change of adding 1 byte gap will break PowerPC memory hotplug in
> its current form.

Sorry should have been explicit in saying that this will break PowerPC
memory hotplug only if gap is enabled like I attempted here:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg00749.html

> 
> Currently we divide hotpluggable memory region into chuncks of 256M
> and there is a DR connector object for each such chunk. The DR connector
> object maintains/controls the state transitions of that memory chunk
> as per PAPR specifications.
> 
> Now after this 1 byte gap, we end up having a 64K alignment (default
> page size) for the DIMM address and end up having an address for which
> there is no DR connector object. I will have to revisit the parts of
> the code in PowerPC that creates DR connector objects and looks them up
> by address.
> 
> Regards,
> Bharata.

Re: [Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to support gaps

[Igor Mammedov]

On Thu, 8 Oct 2015 19:01:09 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Sep 29, 2015 at 04:53:28PM +0200, Igor Mammedov wrote:
> > setting gap to TRUE will make sparse DIMM
> > address auto allocation, leaving gaps between
> > a new DIMM address and preceeding existing DIMM.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   * make 'gap' boolean and just add 1 byte gap
> >     when calculating new_addr for DIMM.
> > ---
> >  hw/i386/pc.c             |  3 ++-
> >  hw/mem/pc-dimm.c         | 15 +++++++++------
> >  hw/ppc/spapr.c           |  2 +-
> >  include/hw/mem/pc-dimm.h |  7 ++++---
> >  4 files changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 461c128..ef02736 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1644,7 +1644,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >          goto out;
> >      }
> > 
> > -    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
> > +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, false,
> > +                        &local_err);
> >      if (local_err) {
> >          goto out;
> >      }
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index bb04862..6cc6ac3 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -32,7 +32,8 @@ typedef struct pc_dimms_capacity {
> >  } pc_dimms_capacity;
> > 
> >  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > -                         MemoryRegion *mr, uint64_t align, Error **errp)
> > +                         MemoryRegion *mr, uint64_t align, bool gap,
> > +                         Error **errp)
> >  {
> >      int slot;
> >      MachineState *machine = MACHINE(qdev_get_machine());
> > @@ -48,7 +49,7 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > 
> >      addr = pc_dimm_get_free_addr(hpms->base,
> >                                   memory_region_size(&hpms->mr),
> > -                                 !addr ? NULL : &addr, align,
> > +                                 !addr ? NULL : &addr, align, gap,
> >                                   memory_region_size(mr), &local_err);
> >      if (local_err) {
> >          goto out;
> > @@ -287,8 +288,8 @@ static int pc_dimm_built_list(Object *obj, void *opaque)
> > 
> >  uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
> >                                 uint64_t address_space_size,
> > -                               uint64_t *hint, uint64_t align, uint64_t size,
> > -                               Error **errp)
> > +                               uint64_t *hint, uint64_t align, bool gap,
> > +                               uint64_t size, Error **errp)
> >  {
> >      GSList *list = NULL, *item;
> >      uint64_t new_addr, ret = 0;
> > @@ -333,13 +334,15 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
> >              goto out;
> >          }
> > 
> > -        if (ranges_overlap(dimm->addr, dimm_size, new_addr, size)) {
> > +        if (ranges_overlap(dimm->addr, dimm_size, new_addr,
> > +                           size + (gap ? 1 : 0))) {
> >              if (hint) {
> >                  DeviceState *d = DEVICE(dimm);
> >                  error_setg(errp, "address range conflicts with '%s'", d->id);
> >                  goto out;
> >              }
> > -            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size, align);
> > +            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size + (gap ? 1 : 0),
> > +                                     align);
> 
> This change of adding 1 byte gap will break PowerPC memory hotplug in
> its current form.
> 
> Currently we divide hotpluggable memory region into chuncks of 256M
> and there is a DR connector object for each such chunk. The DR connector
> object maintains/controls the state transitions of that memory chunk
> as per PAPR specifications.
Originally I've passed gap size explicitly from board to make it more universal,
but was asked to make it this way so now gap is upto backends alignment size
(i.e. for PPC it could endup with 64K or 256M gaps depending on which backend
is used).

If above doesn't fit PPC model well we can switch back to passing gap
size from board so that board would decide what gap size is.

> 
> Now after this 1 byte gap, we end up having a 64K alignment (default
> page size) for the DIMM address and end up having an address for which
> there is no DR connector object. I will have to revisit the parts of
> the code in PowerPC that creates DR connector objects and looks them up
> by address.
> 
> Regards,
> Bharata.
> 

[Qemu-devel] [PATCH v3 2/2] pc: memhp: force gaps between DIMM's GPA

[Igor Mammedov]

mapping DIMMs non contiguously allows to workaround
virtio bug reported earlier:
http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
in this case guest kernel doesn't allocate buffers
that can cross DIMM boundary keeping each buffer
local to a DIMM.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Eduardo Habkost <ehabkost@redhat.com>
---
v3:
  use pc_machine_class_init() for default. (Eduardo)
v2:
  make inter_dimm_gap boolean

benefit of this workaround is that no guest side
changes are required.
---
 hw/i386/pc.c         | 6 ++++--
 hw/i386/pc_piix.c    | 1 +
 hw/i386/pc_q35.c     | 1 +
 include/hw/i386/pc.h | 1 +
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ef02736..efbd41a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1629,6 +1629,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm);
@@ -1644,8 +1645,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, false,
-                        &local_err);
+    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align,
+                        pcmc->inter_dimm_gap, &local_err);
     if (local_err) {
         goto out;
     }
@@ -1946,6 +1947,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
+    pcmc->inter_dimm_gap = true;
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3ffb05f..d97ca2a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->broken_reserved_end = true;
+    pcmc->inter_dimm_gap = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1b7d3b6..c28f0da 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     pc_q35_2_5_machine_options(m);
     m->alias = NULL;
     pcmc->broken_reserved_end = true;
+    pcmc->inter_dimm_gap = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ab5413f..c13e91d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -60,6 +60,7 @@ struct PCMachineClass {
 
     /*< public >*/
     bool broken_reserved_end;
+    bool inter_dimm_gap;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
-- 
1.8.3.1