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 <address@hidden> 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 <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.
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.
>