From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqdfW-0003Z8-Q9 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 05:02:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqdfS-0007cT-No for qemu-devel@nongnu.org; Mon, 26 Oct 2015 05:02:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqdfS-0007cE-I9 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 05:02:14 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 308F58EFC4 for ; Mon, 26 Oct 2015 09:02:14 +0000 (UTC) Date: Mon, 26 Oct 2015 11:02:10 +0200 From: "Michael S. Tsirkin" Message-ID: <20151026105703-mutt-send-email-mst@redhat.com> References: <1445848925-84796-1-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1445848925-84796-1-git-send-email-imammedo@redhat.com> Subject: Re: [Qemu-devel] [PATCH] pc: memhp: enforce minimal 128Mb alignment for pc-dimm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com 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! This is in portable code. Does this imply anyone implementing inter dimm gaps will need the same value? Shouldn't this go into portable code then? > ... > [] acpi_memory_device_remove+0x79/0xa5 > [] acpi_bus_trim+0x5a/0x8d > [] 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 Thanks for the fix. Pls see comments below. > --- > 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 */ > + Pls prefix with PC_ and pls add a comment explaining where does this value come from. > static void pc_dimm_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > @@ -1624,6 +1626,9 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev, > > if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) { > align = memory_region_get_alignment(mr); > + if (pcmc->inter_dimm_gap && (align < MIN_DIMM_ALIGNMENT)) { () not required around math. > + align = MIN_DIMM_ALIGNMENT; > + } This seems wrong. Why is alignment only required when inter_dimm_gap is set? Does this have to do with compatibility somehow? Pls add a comment. > } > > if (!pcms->acpi_dev) { > -- > 1.8.3.1