[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to
From: |
Bharata B Rao |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/2] memhp: extend address auto assignment to support gaps |
Date: |
Thu, 8 Oct 2015 19:01:09 +0530 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 <address@hidden>
> ---
> 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.