Re: [Qemu-devel] [PATCH] pc: memhp: enforce minimal 128Mb alignment for pc-dimm

[Eduardo Habkost]

On Tue, Oct 27, 2015 at 11:08:26AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 26, 2015 at 04:33:18PM -0200, Eduardo Habkost wrote:
> > On Mon, Oct 26, 2015 at 09:42:05AM +0100, Igor Mammedov wrote:
> > > commit aa8580cd "pc: memhp: force gaps between DIMM's GPA"
> > > regressed memory hot-unplug for linux guests triggering
> > > following BUGON
> > >  =====
> > >  kernel BUG at mm/memory_hotplug.c:703!
> > >  ...
> > >  [<ffffffff81385fa7>] acpi_memory_device_remove+0x79/0xa5
> > >  [<ffffffff81357818>] acpi_bus_trim+0x5a/0x8d
> > >  [<ffffffff81359026>] acpi_device_hotplug+0x1b7/0x418
> > >  ===
> > >     BUG_ON(phys_start_pfn & ~PAGE_SECTION_MASK);
> > >  ===
> > > 
> > > reson for it is that x86-64 linux guest supports memory
> > > hotplug in chunks of 128Mb and memory section also should
> > > be 128Mb aligned.
> > > However gaps forced between 128Mb DIMMs with backend's
> > > natural alignment of 2Mb make the 2nd and following
> > > DIMMs not being aligned on 128Mb boundary as it was
> > > originally. To fix regression enforce minimal 128Mb
> > > alignment like it was done for PPC.
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > >  hw/i386/pc.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 3d958ba..cd68169 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1610,6 +1610,8 @@ void ioapic_init_gsi(GSIState *gsi_state, const 
> > > char *parent_name)
> > >      }
> > >  }
> > >  
> > > +#define MIN_DIMM_ALIGNMENT (1ULL << 27) /* 128Mb */
> > 
> > If you send a new version, could you include the explanation for the
> > 128MB value as a comment above the macro definition?
> 
> The issue is that there's no good explanation yet.  It's just something
> that seems to work for current linux.  Why does linux do it, and what
> basis does it have in hardware, IIUC we don't know.

We just need an explanation to why we chose that value, even if we don't
know yet why it works. Even "this is the only value we ever tested and
it seems to work, good luck figuring out why" would be better than no
explanation, IMO.

-- 
Eduardo