Re: [Qemu-devel] [PATCH] pc: memhotplug: rise minimum DIMM addr/size alignment to 128Mb

[Paolo Bonzini]

On 21/09/2015 13:50, Igor Mammedov wrote:
> it's attempt to workaround virtio bug reported earlier:
> http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
> where virtio can't handle buffer that crosses border
> between 2 DIMM's (i.e. 2 MemoryRegions).
> 
> Testing showed that virtio doesn't hit above bug
> with 128Mb DIMM's granularity. Also linux memory
> hotplug can handle hotplugged memory starting with
> 128Mb memory sections so lets rise minimum size limit
> to 128Mb and align starting DIMM address on 128Mb.
> 
> It's certainly not the fix but it reduces risk of
> crashing VM till virtio is fixed.
> It also could be improved in guest's virtio side if it
> would align buffers on 128Mb border and limit max  buffer
> size to the same value.
> 
> Signed-off-by: Igor Mammedov <address@hidden>

This seems to be easily handled at a level above QEMU---and the fix
would be available to older machine types as well.  This patch would
also make it quite a bit harder to test the real fix with QEMU. It is
not alone a reason to NACK it but should also be kept in mind.

Aligning to 4K makes some sense, since 4K is the page size, but
enforcing an arbitrary alignment above 4K is policy that does not belong
in QEMU.

To some extend, enforcing natural alignment would be okay as a
workaround for the virtio bug as well.  It would also make it easier to
ensure that hotplugged hugetlbfs-backed memory can use hugepages in the
guest.  Does it make sense to you?

Paolo

> ---
> Based on PCI tree as it has patches that add
> 2.5 machine type.
> ---
>  hw/i386/pc.c         |  8 +++++---
>  hw/i386/pc_piix.c    | 12 ++++++++++--
>  hw/i386/pc_q35.c     | 12 ++++++++++--
>  include/hw/i386/pc.h |  5 ++---
>  4 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b5107f7..ddb6710 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1645,8 +1645,9 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t align = TARGET_PAGE_SIZE;
>  
> -    if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
> -        align = memory_region_get_alignment(mr);
> +    if (pcms->enforce_aligned_dimm) {
> +        align = MAX(memory_region_get_alignment(mr),
> +                    pcms->enforce_aligned_dimm);
>      }
>  
>      if (!pcms->acpi_dev) {
> @@ -1936,7 +1937,8 @@ static void pc_machine_initfn(Object *obj)
>                                      "Enable vmport (pc & q35)",
>                                      &error_abort);
>  
> -    pcms->enforce_aligned_dimm = true;
> +    /* align DIMM starting address/size by 128Mb */
> +    pcms->enforce_aligned_dimm = 1ULL << 27;
>      object_property_add_bool(obj, PC_MACHINE_ENFORCE_ALIGNED_DIMM,
>                               pc_machine_get_aligned_dimm,
>                               NULL, &error_abort);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index caa4edc..7671905 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -301,9 +301,17 @@ static void pc_init1(MachineState *machine,
>      }
>  }
>  
> +static void pc_compat_2_4(MachineState *machine)
> +{
> +    PCMachineState *pcms = PC_MACHINE(machine);
> +
> +    pcms->enforce_aligned_dimm = TARGET_PAGE_SIZE;
> +}
> +
>  static void pc_compat_2_3(MachineState *machine)
>  {
>      PCMachineState *pcms = PC_MACHINE(machine);
> +    pc_compat_2_4(machine);
>      savevm_skip_section_footers();
>      if (kvm_enabled()) {
>          pcms->smm = ON_OFF_AUTO_OFF;
> @@ -326,7 +334,7 @@ static void pc_compat_2_1(MachineState *machine)
>      pc_compat_2_2(machine);
>      smbios_uuid_encoded = false;
>      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> -    pcms->enforce_aligned_dimm = false;
> +    pcms->enforce_aligned_dimm = 0;
>  }
>  
>  static void pc_compat_2_0(MachineState *machine)
> @@ -485,7 +493,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", NULL,
> +DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", pc_compat_2_3,
>                        pc_i440fx_2_4_machine_options)
>  
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 506b6bf..72b479f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -284,9 +284,17 @@ static void pc_q35_init(MachineState *machine)
>      }
>  }
>  
> +static void pc_compat_2_4(MachineState *machine)
> +{
> +    PCMachineState *pcms = PC_MACHINE(machine);
> +
> +    pcms->enforce_aligned_dimm = TARGET_PAGE_SIZE;
> +}
> +
>  static void pc_compat_2_3(MachineState *machine)
>  {
>      PCMachineState *pcms = PC_MACHINE(machine);
> +    pc_compat_2_4(machine);
>      savevm_skip_section_footers();
>      if (kvm_enabled()) {
>          pcms->smm = ON_OFF_AUTO_OFF;
> @@ -307,7 +315,7 @@ static void pc_compat_2_1(MachineState *machine)
>      PCMachineState *pcms = PC_MACHINE(machine);
>  
>      pc_compat_2_2(machine);
> -    pcms->enforce_aligned_dimm = false;
> +    pcms->enforce_aligned_dimm = 0;
>      smbios_uuid_encoded = false;
>      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
>  }
> @@ -388,7 +396,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", NULL,
> +DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", pc_compat_2_4,
>                     pc_q35_2_4_machine_options);
>  
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 6896328..fdcf0ec 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -23,8 +23,7 @@
>  /**
>   * PCMachineState:
>   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> - * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
> - *                        backend's alignment value if provided
> + * @enforce_aligned_dimm: minimal DIMM's address/size alignment
>   */
>  struct PCMachineState {
>      /*< private >*/
> @@ -37,9 +36,9 @@ struct PCMachineState {
>      ISADevice *rtc;
>  
>      uint64_t max_ram_below_4g;
> +    uint64_t enforce_aligned_dimm;
>      OnOffAuto vmport;
>      OnOffAuto smm;
> -    bool enforce_aligned_dimm;
>      ram_addr_t below_4g_mem_size, above_4g_mem_size;
>  };
>  
>