[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pc: memhp: enforce minimal 128Mb alignment for
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH] pc: memhp: enforce minimal 128Mb alignment for pc-dimm |
Date: |
Mon, 26 Oct 2015 10:20:09 +0100 |
On Mon, 26 Oct 2015 11:02:10 +0200
"Michael S. Tsirkin" <address@hidden> 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!
>
> 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?
yep, but PAGE_SECTION_MASK => secstion size is not portable
(i.e. it's per target define)
>
> > ...
> > [<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>
>
>
> 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.
sure
>
> > 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.
indeed, it's keyed on inter_dimm_gap for compatibility reasons.
and since inter_dimm_gap introduced layout change it should be ok
to make fix also depend on inter_dimm_gap and not to touch previous machine
types.
I'll respin v2.
>
> > }
> >
> > if (!pcms->acpi_dev) {
> > --
> > 1.8.3.1