On Mon, 21 Sep 2015 14:22:23 +0200 Paolo Bonzini <pbonz...@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 <imamm...@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. > > 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; > > }; > > > >