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

[Michael S. Tsirkin]

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?

>  ...
>  [<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.

>  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