[Qemu-devel] [RFC v0 0/2] Enforce gaps between DIMMs

[Qemu-devel] [RFC v0 0/2] Enforce gaps between DIMMs

[Bharata B Rao]

The suggested way to work around the virtio bug reported here

http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html

is to introduce gaps between DIMMs. Igor's patchset changes the pc-dimm
auto-address assignment to introduce gaps and ues the same from pc memhp.
This patchset does the same for sPAPR PowerPC.

Before introducing the gap, ensure that memory hotplug region has enough
room for alignment adjustment. We accommodate a max alignment of 256MB for
each slot since sPAPR memory hotplug enforces an alignment requirement of
256MB on RAM size, maxmem and NUMA node mem sizes.

This applies on David's spapr-next branch + Igor's patchset applied.

This has been very lightly tested and intention is to get feedback
on the correctness aspect of this.

Bharata B Rao (2):
  spapr: Accommadate alignment gaps in hotplug memory region
  spapr: Force gaps between DIMM's GPA

 hw/ppc/spapr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.1.0

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

[Bharata B Rao]

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;
         memory_region_init(&spapr->hotplug_memory.mr, OBJECT(spapr),
                            "hotplug-memory", hotplug_mem_size);
         memory_region_add_subregion(sysmem, spapr->hotplug_memory.base,
-- 
2.1.0

Re: [Qemu-devel] [RFC v0 1/2] spapr: Accommadate alignment gaps in hotplug memory region

[Igor Mammedov]

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,

Re: [Qemu-devel] [RFC v0 1/2] spapr: Accommadate alignment gaps in hotplug memory region

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC v0 1/2] spapr: Accommadate alignment gaps in hotplug memory region

[Igor Mammedov]

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.

Re: [Qemu-devel] [RFC v0 1/2] spapr: Accommadate alignment gaps in hotplug memory region

[Thomas Huth]

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

[Qemu-devel] [RFC v0 2/2] spapr: Force gaps between DIMM's GPA

[Bharata B Rao]

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: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2ec509b..a8526e9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2127,7 +2127,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, false, &local_err);
+    pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, true, &local_err);
     if (local_err) {
         goto out;
     }
-- 
2.1.0

Re: [Qemu-devel] [RFC v0 0/2] Enforce gaps between DIMMs

[Michael S. Tsirkin]

On Mon, Oct 05, 2015 at 02:05:22PM +0530, Bharata B Rao wrote:
> The suggested way to work around the virtio bug reported here
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
> 
> is to introduce gaps between DIMMs. Igor's patchset changes the pc-dimm
> auto-address assignment to introduce gaps and ues the same from pc memhp.
> This patchset does the same for sPAPR PowerPC.
> 
> Before introducing the gap, ensure that memory hotplug region has enough
> room for alignment adjustment. We accommodate a max alignment of 256MB for
> each slot since sPAPR memory hotplug enforces an alignment requirement of
> 256MB on RAM size, maxmem and NUMA node mem sizes.
> 
> This applies on David's spapr-next branch + Igor's patchset applied.
> 
> This has been very lightly tested and intention is to get feedback
> on the correctness aspect of this.
> 
> Bharata B Rao (2):
>   spapr: Accommadate alignment gaps in hotplug memory region
>   spapr: Force gaps between DIMM's GPA

PC needs this, PPC needs this ... I don't see why would this not
apply everywhere. Isn't it time we just converted everyone?
Drop the gap flag, set it unconditionally for new machine types.

Thoughts?


>  hw/ppc/spapr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [RFC v0 0/2] Enforce gaps between DIMMs

[Igor Mammedov]

On Mon, 5 Oct 2015 23:24:33 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Oct 05, 2015 at 02:05:22PM +0530, Bharata B Rao wrote:
> > The suggested way to work around the virtio bug reported here
> > 
> > http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
> > 
> > is to introduce gaps between DIMMs. Igor's patchset changes the pc-dimm
> > auto-address assignment to introduce gaps and ues the same from pc memhp.
> > This patchset does the same for sPAPR PowerPC.
> > 
> > Before introducing the gap, ensure that memory hotplug region has enough
> > room for alignment adjustment. We accommodate a max alignment of 256MB for
> > each slot since sPAPR memory hotplug enforces an alignment requirement of
> > 256MB on RAM size, maxmem and NUMA node mem sizes.
> > 
> > This applies on David's spapr-next branch + Igor's patchset applied.
> > 
> > This has been very lightly tested and intention is to get feedback
> > on the correctness aspect of this.
> > 
> > Bharata B Rao (2):
> >   spapr: Accommadate alignment gaps in hotplug memory region
> >   spapr: Force gaps between DIMM's GPA
> 
> PC needs this, PPC needs this ... I don't see why would this not
> apply everywhere. Isn't it time we just converted everyone?
> Drop the gap flag, set it unconditionally for new machine types.
I guess that flag is needed exactly for compatibility with old
machine types which are per board thingy.

BTW:
does PPC have machine types? I don't see any compatibility
code in this series?

> 
> Thoughts?
> 
> 
> >  hw/ppc/spapr.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > -- 
> > 2.1.0
> > 

Re: [Qemu-devel] [RFC v0 0/2] Enforce gaps between DIMMs

[Bharata B Rao]

On Tue, Oct 06, 2015 at 10:08:16AM +0200, Igor Mammedov wrote:
> On Mon, 5 Oct 2015 23:24:33 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Oct 05, 2015 at 02:05:22PM +0530, Bharata B Rao wrote:
> > > The suggested way to work around the virtio bug reported here
> > > 
> > > http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
> > > 
> > > is to introduce gaps between DIMMs. Igor's patchset changes the pc-dimm
> > > auto-address assignment to introduce gaps and ues the same from pc memhp.
> > > This patchset does the same for sPAPR PowerPC.
> > > 
> > > Before introducing the gap, ensure that memory hotplug region has enough
> > > room for alignment adjustment. We accommodate a max alignment of 256MB for
> > > each slot since sPAPR memory hotplug enforces an alignment requirement of
> > > 256MB on RAM size, maxmem and NUMA node mem sizes.
> > > 
> > > This applies on David's spapr-next branch + Igor's patchset applied.
> > > 
> > > This has been very lightly tested and intention is to get feedback
> > > on the correctness aspect of this.
> > > 
> > > Bharata B Rao (2):
> > >   spapr: Accommadate alignment gaps in hotplug memory region
> > >   spapr: Force gaps between DIMM's GPA
> > 
> > PC needs this, PPC needs this ... I don't see why would this not
> > apply everywhere. Isn't it time we just converted everyone?
> > Drop the gap flag, set it unconditionally for new machine types.
> I guess that flag is needed exactly for compatibility with old
> machine types which are per board thingy.
> 
> BTW:
> does PPC have machine types? I don't see any compatibility
> code in this series?

For PPC, memory hotplug support starts from machine type pseries-2.5
which is the latest and hence we haven't had a need to be compatible
with any other machine type till now.

Regards,
Bharata.