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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 21 Sep 2015 14:22:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> 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 <imammedo@redhat.com>
> 
> 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.
Patch makes it easy to change enforced alignment for future machine types,
so lowering alignment for testing isn't hard when virtio is fixed.

Handling it at libvirt level is a bit hard since currently it doesn't
deal with DIMM.addr allocation and there isn't any interface
to communicate hotplug address range to libvirt. Libvirt doesn't need
to know anything about DIMM.addr except of migration when it needs
to replicate state on target side.

Also it's QEMU bug/fault and pushing workaround to upper layers
doesn't seem right when it's much easier to do it in QEMU itself.


> 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;
> >  };
> >  
> > 
>