[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pc: memhotplug: rise minimum DIMM addr/size ali
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH] pc: memhotplug: rise minimum DIMM addr/size alignment to 128Mb |
Date: |
Mon, 21 Sep 2015 15:05:24 +0200 |
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;
> > };
> >
> >
Re: [Qemu-devel] [PATCH] pc: memhotplug: rise minimum DIMM addr/size alignment to 128Mb, Eduardo Habkost, 2015/09/21