[RFC,v0,1/2] spapr: Accommadate alignment gaps in hotplug memory region

Size hotplug memory region assuming a 256MB max alignment every slot.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 3 +++
 1 file changed, 3 insertions(+)

On Mon,  5 Oct 2015 14:05:23 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Size hotplug memory region assuming a 256MB max alignment every slot.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fc5e7d6..2ec509b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1792,6 +1792,9 @@ static void ppc_spapr_init(MachineState *machine)
>  
>          spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
>                                                SPAPR_HOTPLUG_MEM_ALIGN);
> +
> +        /* size hotplug region assuming 256M max alignment per slot */
> +        hotplug_mem_size += SPAPR_MEMORY_BLOCK_SIZE * machine->ram_slots;
Does target support hugepages backend? If it does then adjustment probably
should be max supported hugepage alignment.

>          memory_region_init(&spapr->hotplug_memory.mr, OBJECT(spapr),
>                             "hotplug-memory", hotplug_mem_size);
>          memory_region_add_subregion(sysmem, spapr->hotplug_memory.base,

On 05/10/15 10:35, Bharata B Rao wrote:
> Size hotplug memory region assuming a 256MB max alignment every slot.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fc5e7d6..2ec509b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1792,6 +1792,9 @@ static void ppc_spapr_init(MachineState *machine)
>  
>          spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
>                                                SPAPR_HOTPLUG_MEM_ALIGN);
> +
> +        /* size hotplug region assuming 256M max alignment per slot */
> +        hotplug_mem_size += SPAPR_MEMORY_BLOCK_SIZE * machine->ram_slots;

Could you maybe make the comment here a little bit more verbose? Without
reading the cover letter first, I would not understand this piece of
code ... and I'm afraid that when we look at this code in a couple of
years again, we hit the same problem when the comment is that short.

Thanks,
  Thomas

On Mon, Oct 05, 2015 at 11:05:07AM +0200, Igor Mammedov wrote:
> On Mon,  5 Oct 2015 14:05:23 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Size hotplug memory region assuming a 256MB max alignment every slot.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index fc5e7d6..2ec509b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1792,6 +1792,9 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >          spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
> >                                                SPAPR_HOTPLUG_MEM_ALIGN);
> > +
> > +        /* size hotplug region assuming 256M max alignment per slot */
> > +        hotplug_mem_size += SPAPR_MEMORY_BLOCK_SIZE * machine->ram_slots;
> Does target support hugepages backend? If it does then adjustment probably
> should be max supported hugepage alignment.

Hrm, so the maximum possible page size on Power is 16G (though we
don't yet support that on "powernv" which is what the host system will
generally be).

Not sure if the possibility of 16G "colossal pages" in future is
enough reason to put such a huge gap.  There aren't any other page
sizes between 16MB and 16GB.

On Tue, 6 Oct 2015 14:29:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Oct 05, 2015 at 11:05:07AM +0200, Igor Mammedov wrote:
> > On Mon,  5 Oct 2015 14:05:23 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Size hotplug memory region assuming a 256MB max alignment every slot.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index fc5e7d6..2ec509b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1792,6 +1792,9 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >          spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
> > >                                                SPAPR_HOTPLUG_MEM_ALIGN);
> > > +
> > > +        /* size hotplug region assuming 256M max alignment per slot */
> > > +        hotplug_mem_size += SPAPR_MEMORY_BLOCK_SIZE * machine->ram_slots;
> > Does target support hugepages backend? If it does then adjustment probably
> > should be max supported hugepage alignment.
> 
> Hrm, so the maximum possible page size on Power is 16G (though we
> don't yet support that on "powernv" which is what the host system will
> generally be).
> 
> Not sure if the possibility of 16G "colossal pages" in future is
> enough reason to put such a huge gap.  There aren't any other page
> sizes between 16MB and 16GB.

Perhaps add a comment and a more verbose description in commit message
about this so that in future not to wonder why 256 has been chosen.