| Message ID | 1443538409-145558-2-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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.
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.
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. >
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
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(-)