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

[Igor Mammedov]

On Mon, 21 Sep 2015 14:22:23 +0200
Paolo Bonzini <address@hidden> 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 <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?
in current machine types we already enforce backend-s address/size alignment,
which is file's page size for hugetlbfs-backed memory and 2Mb for RAM backend.

So I guess we could try to apply workaround to virtio on guest side,
aligning and limiting max buffer size to 2Mb, it should work for 'old'
machine types as well.

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