| Message ID | 20230921071755.61081-1-anisinha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] hw/i386/pc: improve physical address space bound check for 32-bit systems | expand |
> On 21-Sep-2023, at 12:47 PM, Ani Sinha <anisinha@redhat.com> wrote: > > 32-bit systems do not have a reserved memory for hole64 and hotplugging memory > devices are not supported on those systems. Relevant thread for Linux kernel https://lore.kernel.org/all/20210929143600.49379-4-david@redhat.com/ > Therefore, the maximum limit of the > guest physical address in the absence of additional memory devices effectively > coincides with the end of "above 4G memory space" region. When users configure > additional memory devices, we need to properly account for the additional device > memory region so as to find the maximum value of the guest physical address > and enforce that it is within the physical address space of the processor. For > 32-bit, this maximum PA will be outside the range of the processor's address > space. > > With this change, for example, previously this was allowed: > > $ ./qemu-system-x86_64 -cpu pentium -m size=10G > > Now it is no longer allowed: > > $ ./qemu-system-x86_64 -cpu pentium -m size=10G > qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff phys-bits too low (32) > > For 32-bit, hotplugging additional memory is no longer allowed. > > $ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 > qemu-system-i386: Address space limit 0xffffffff < 0x1ffffffff phys-bits too low (32) > > The above is still allowed for older machine types in order to support > compatibility. Therefore, this still works: > > $ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2 > > After calling CPUID with EAX=0x80000001, all AMD64 compliant processors > have the longmode-capable-bit turned on in the extended feature flags (bit 29) > in EDX. The absence of CPUID longmode can be used to differentiate between > 32-bit and 64-bit processors and is the recommended approach. QEMU takes this > approach elsewhere (for example, please see x86_cpu_realizefn()) and with > this change, pc_max_used_gpa() also takes the same approach to detect 32-bit > processors. > > Unit tests are modified to not run those tests that use memory hotplug > on 32-bit x86 architecture. > > Finally, a new compatibility flag is introduced to retain the old behavior > for pc_max_used_gpa() for machines 8.1 and older. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Ani Sinha <anisinha@redhat.com> > --- > hw/i386/pc.c | 34 +++++++++++++++++++++++++++++++--- > hw/i386/pc_piix.c | 4 ++++ > include/hw/i386/pc.h | 3 +++ > tests/qtest/bios-tables-test.c | 26 ++++++++++++++++++-------- > tests/qtest/numa-test.c | 7 ++++++- > 5 files changed, 62 insertions(+), 12 deletions(-) > > changelog: > v3: still accounting for additional memory device region above 4G. > unit tests fixed (not running for 32-bit where mem hotplug is used). > > v2: removed memory hotplug region from max_gpa. added compat knobs. > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 54838c0c41..0aa2f6b6c0 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -907,12 +907,39 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms) > static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size) > { > X86CPU *cpu = X86_CPU(first_cpu); > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > + MachineState *ms = MACHINE(pcms); > + uint64_t devmem_start = 0; > + ram_addr_t devmem_size = 0; > > - /* 32-bit systems don't have hole64 thus return max CPU address */ > - if (cpu->phys_bits <= 32) { > - return ((hwaddr)1 << cpu->phys_bits) - 1; > + /* > + * 32-bit systems don't have hole64 but they might have a region for > + * memory devices. Even if additional hotplugged memory devices might > + * not be usable by most guest OSes, we need to still consider them for > + * calculating the highest possible GPA so that we can properly report > + * if someone configures them on a CPU that cannot possibly address them. > + */ > + if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) { > + /* 32-bit systems */ > + if (pcmc->fixed_32bit_mem_addr_check) { > + if (pcmc->has_reserved_memory && > + (ms->ram_size < ms->maxram_size)) { > + pc_get_device_memory_range(pcms, &devmem_start, > + &devmem_size); > + if (!pcmc->broken_reserved_end) { > + devmem_start += devmem_size; > + } > + return devmem_start - 1; > + } else { > + return pc_above_4g_end(pcms) - 1; > + } > + } else { > + /* old value for compatibility reasons */ > + return ((hwaddr)1 << cpu->phys_bits) - 1; > + } > } > > + /* 64-bit systems */ > return pc_pci_hole64_start() + pci_hole64_size - 1; > } > > @@ -1867,6 +1894,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > pcmc->pvh_enabled = true; > pcmc->kvmclock_create_always = true; > pcmc->resizable_acpi_blob = true; > + pcmc->fixed_32bit_mem_addr_check = true; > assert(!mc->get_hotplug_handler); > mc->get_hotplug_handler = pc_get_hotplug_handler; > mc->hotplug_allowed = pc_hotplug_allowed; > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 8321f36f97..f100a5de8b 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -517,9 +517,13 @@ DEFINE_I440FX_MACHINE(v8_2, "pc-i440fx-8.2", NULL, > > static void pc_i440fx_8_1_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > + > pc_i440fx_8_2_machine_options(m); > m->alias = NULL; > m->is_default = false; > + pcmc->fixed_32bit_mem_addr_check = false; > + > compat_props_add(m->compat_props, hw_compat_8_1, hw_compat_8_1_len); > compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len); > } > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 0fabece236..5a70d163d0 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -129,6 +129,9 @@ struct PCMachineClass { > > /* resizable acpi blob compat */ > bool resizable_acpi_blob; > + > + /* fixed 32-bit processor address space bound check for memory */ > + bool fixed_32bit_mem_addr_check; > }; > > #define TYPE_PC_MACHINE "generic-pc-machine" > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index d1b80149f2..f8e03dfd46 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -2080,7 +2080,6 @@ int main(int argc, char *argv[]) > test_acpi_piix4_no_acpi_pci_hotplug); > qtest_add_func("acpi/piix4/ipmi", test_acpi_piix4_tcg_ipmi); > qtest_add_func("acpi/piix4/cpuhp", test_acpi_piix4_tcg_cphp); > - qtest_add_func("acpi/piix4/memhp", test_acpi_piix4_tcg_memhp); > qtest_add_func("acpi/piix4/numamem", test_acpi_piix4_tcg_numamem); > qtest_add_func("acpi/piix4/nosmm", test_acpi_piix4_tcg_nosmm); > qtest_add_func("acpi/piix4/smm-compat", > @@ -2088,9 +2087,15 @@ int main(int argc, char *argv[]) > qtest_add_func("acpi/piix4/smm-compat-nosmm", > test_acpi_piix4_tcg_smm_compat_nosmm); > qtest_add_func("acpi/piix4/nohpet", test_acpi_piix4_tcg_nohpet); > - qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm); > - qtest_add_func("acpi/piix4/acpihmat", > - test_acpi_piix4_tcg_acpi_hmat); > + > + /* i386 does not support memory hotplug */ > + if (strcmp(arch, "i386")) { > + qtest_add_func("acpi/piix4/memhp", test_acpi_piix4_tcg_memhp); > + qtest_add_func("acpi/piix4/dimmpxm", > + test_acpi_piix4_tcg_dimm_pxm); > + qtest_add_func("acpi/piix4/acpihmat", > + test_acpi_piix4_tcg_acpi_hmat); > + } > #ifdef CONFIG_POSIX > qtest_add_func("acpi/piix4/acpierst", test_acpi_piix4_acpi_erst); > #endif > @@ -2108,11 +2113,9 @@ int main(int argc, char *argv[]) > test_acpi_q35_tcg_no_acpi_hotplug); > qtest_add_func("acpi/q35/multif-bridge", > test_acpi_q35_multif_bridge); > - qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); > qtest_add_func("acpi/q35/ipmi", test_acpi_q35_tcg_ipmi); > qtest_add_func("acpi/q35/smbus/ipmi", test_acpi_q35_tcg_smbus_ipmi); > qtest_add_func("acpi/q35/cpuhp", test_acpi_q35_tcg_cphp); > - qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp); > qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem); > qtest_add_func("acpi/q35/nosmm", test_acpi_q35_tcg_nosmm); > qtest_add_func("acpi/q35/smm-compat", > @@ -2120,10 +2123,17 @@ int main(int argc, char *argv[]) > qtest_add_func("acpi/q35/smm-compat-nosmm", > test_acpi_q35_tcg_smm_compat_nosmm); > qtest_add_func("acpi/q35/nohpet", test_acpi_q35_tcg_nohpet); > - qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm); > - qtest_add_func("acpi/q35/acpihmat", test_acpi_q35_tcg_acpi_hmat); > qtest_add_func("acpi/q35/acpihmat-noinitiator", > test_acpi_q35_tcg_acpi_hmat_noinitiator); > + > + /* i386 does not support memory hotplug */ > + if (strcmp(arch, "i386")) { > + qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp); > + qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm); > + qtest_add_func("acpi/q35/acpihmat", > + test_acpi_q35_tcg_acpi_hmat); > + qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); > + } > #ifdef CONFIG_POSIX > qtest_add_func("acpi/q35/acpierst", test_acpi_q35_acpi_erst); > #endif > diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c > index c5eb13f349..4f4404a4b1 100644 > --- a/tests/qtest/numa-test.c > +++ b/tests/qtest/numa-test.c > @@ -568,7 +568,7 @@ int main(int argc, char **argv) > qtest_add_data_func("/numa/mon/cpus/partial", args, test_mon_partial); > qtest_add_data_func("/numa/qmp/cpus/query-cpus", args, test_query_cpus); > > - if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) { > + if (!strcmp(arch, "x86_64")) { > qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu); > qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg); > qtest_add_data_func("/numa/pc/hmat/build", args, pc_hmat_build_cfg); > @@ -576,6 +576,11 @@ int main(int argc, char **argv) > qtest_add_data_func("/numa/pc/hmat/erange", args, pc_hmat_erange_cfg); > } > > + if (!strcmp(arch, "i386")) { > + qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu); > + qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg); > + } > + > if (!strcmp(arch, "ppc64")) { > qtest_add_data_func("/numa/spapr/cpu/explicit", args, spapr_numa_cpu); > } > -- > 2.39.1 >
On 21.09.23 09:17, Ani Sinha wrote: > 32-bit systems do not have a reserved memory for hole64 and hotplugging memory > devices are not supported on those systems. Therefore, the maximum limit of the > guest physical address in the absence of additional memory devices effectively > coincides with the end of "above 4G memory space" region. When users configure > additional memory devices, we need to properly account for the additional device > memory region so as to find the maximum value of the guest physical address > and enforce that it is within the physical address space of the processor. For > 32-bit, this maximum PA will be outside the range of the processor's address > space. > > With this change, for example, previously this was allowed: > > $ ./qemu-system-x86_64 -cpu pentium -m size=10G > > Now it is no longer allowed: > > $ ./qemu-system-x86_64 -cpu pentium -m size=10G > qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff phys-bits too low (32) > > For 32-bit, hotplugging additional memory is no longer allowed. "32-bit without PAE/PSE36" > > $ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 > qemu-system-i386: Address space limit 0xffffffff < 0x1ffffffff phys-bits too low (32) > We always place the device memory region above 4G. Without PAE/PSE36, you cannot ever possibly make use hotplugged memory, because it would reside > 4g. So while the user could have started that QEMU instance, even with an OS that would support memory hotplug, a DIMM above 4G would not have been usable. So we're now properly failing for a setup that doesn't make any sense. Good :) ... if someone ever cares about making that work, we would have to let the device memory region start below 4g (and obviously, not exceed 4g). So while ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 fails (because pentium cannot access that memory), what should work is ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 -cpu pentium2 or ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 -cpu pentium,pse36=on Because that CPU could actually address that memory somehow (PAE/PSE36). So IMHO, we're now forbidding setups that are impossible. > The above is still allowed for older machine types in order to support > compatibility. Therefore, this still works: > > $ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2 > Makes sense. (probably nobody cares, but better safe than sorry) > After calling CPUID with EAX=0x80000001, all AMD64 compliant processors > have the longmode-capable-bit turned on in the extended feature flags (bit 29) > in EDX. The absence of CPUID longmode can be used to differentiate between > 32-bit and 64-bit processors and is the recommended approach. QEMU takes this > approach elsewhere (for example, please see x86_cpu_realizefn()) and with > this change, pc_max_used_gpa() also takes the same approach to detect 32-bit > processors. > > Unit tests are modified to not run those tests that use memory hotplug > on 32-bit x86 architecture. We could use a different CPU (pentium2) to still run these tests. "pentium2" should work I assume? [...] > @@ -907,12 +907,39 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms) > static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size) > { > X86CPU *cpu = X86_CPU(first_cpu); > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > + MachineState *ms = MACHINE(pcms); > + uint64_t devmem_start = 0; > + ram_addr_t devmem_size = 0; > > - /* 32-bit systems don't have hole64 thus return max CPU address */ > - if (cpu->phys_bits <= 32) { > - return ((hwaddr)1 << cpu->phys_bits) - 1; > + /* > + * 32-bit systems don't have hole64 but they might have a region for > + * memory devices. Even if additional hotplugged memory devices might > + * not be usable by most guest OSes, we need to still consider them for > + * calculating the highest possible GPA so that we can properly report > + * if someone configures them on a CPU that cannot possibly address them. > + */ > + if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) { > + /* 32-bit systems */ > + if (pcmc->fixed_32bit_mem_addr_check) { > + if (pcmc->has_reserved_memory && > + (ms->ram_size < ms->maxram_size)) { > + pc_get_device_memory_range(pcms, &devmem_start, > + &devmem_size); > + if (!pcmc->broken_reserved_end) { I think you can remove that check. "pcmc->fixed_32bit_mem_addr_check && pcmc->broken_reserved_end" can never hold at the same time. broken_reserved_end is only set for QEMU <= 2.4, to work around another broken check. pcmc->fixed_32bit_mem_addr_check is only set for 8.2+. Maybe consider calling "fixed_32bit_mem_addr_check" "pcmc->broken_32bit_max_gpa_check" and reverse the logic (treating it like broken_reserved_end).
> On 21-Sep-2023, at 12:47 PM, Ani Sinha <anisinha@redhat.com> wrote: > > 32-bit systems do not have a reserved memory for hole64 and hotplugging memory > devices are not supported on those systems. Therefore, the maximum limit of the > guest physical address in the absence of additional memory devices effectively > coincides with the end of "above 4G memory space" region. When users configure > additional memory devices, we need to properly account for the additional device > memory region so as to find the maximum value of the guest physical address > and enforce that it is within the physical address space of the processor. For > 32-bit, this maximum PA will be outside the range of the processor's address > space. > > With this change, for example, previously this was allowed: > > $ ./qemu-system-x86_64 -cpu pentium -m size=10G > > Now it is no longer allowed: > > $ ./qemu-system-x86_64 -cpu pentium -m size=10G > qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff phys-bits too low (32) > > For 32-bit, hotplugging additional memory is no longer allowed. > > $ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 > qemu-system-i386: Address space limit 0xffffffff < 0x1ffffffff phys-bits too low (32) > > The above is still allowed for older machine types in order to support > compatibility. Therefore, this still works: > > $ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2 > > After calling CPUID with EAX=0x80000001, all AMD64 compliant processors > have the longmode-capable-bit turned on in the extended feature flags (bit 29) > in EDX. The absence of CPUID longmode can be used to differentiate between > 32-bit and 64-bit processors and is the recommended approach. QEMU takes this > approach elsewhere (for example, please see x86_cpu_realizefn()) and with > this change, pc_max_used_gpa() also takes the same approach to detect 32-bit > processors. > > Unit tests are modified to not run those tests that use memory hotplug > on 32-bit x86 architecture. > > Finally, a new compatibility flag is introduced to retain the old behavior > for pc_max_used_gpa() for machines 8.1 and older. This patch has a bug. I forgot to add the knob for older q35 machines. Will fix in next version. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Ani Sinha <anisinha@redhat.com> > --- > hw/i386/pc.c | 34 +++++++++++++++++++++++++++++++--- > hw/i386/pc_piix.c | 4 ++++ > include/hw/i386/pc.h | 3 +++ > tests/qtest/bios-tables-test.c | 26 ++++++++++++++++++-------- > tests/qtest/numa-test.c | 7 ++++++- > 5 files changed, 62 insertions(+), 12 deletions(-) > > changelog: > v3: still accounting for additional memory device region above 4G. > unit tests fixed (not running for 32-bit where mem hotplug is used). > > v2: removed memory hotplug region from max_gpa. added compat knobs. > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 54838c0c41..0aa2f6b6c0 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -907,12 +907,39 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms) > static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size) > { > X86CPU *cpu = X86_CPU(first_cpu); > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > + MachineState *ms = MACHINE(pcms); > + uint64_t devmem_start = 0; > + ram_addr_t devmem_size = 0; > > - /* 32-bit systems don't have hole64 thus return max CPU address */ > - if (cpu->phys_bits <= 32) { > - return ((hwaddr)1 << cpu->phys_bits) - 1; > + /* > + * 32-bit systems don't have hole64 but they might have a region for > + * memory devices. Even if additional hotplugged memory devices might > + * not be usable by most guest OSes, we need to still consider them for > + * calculating the highest possible GPA so that we can properly report > + * if someone configures them on a CPU that cannot possibly address them. > + */ > + if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) { > + /* 32-bit systems */ > + if (pcmc->fixed_32bit_mem_addr_check) { > + if (pcmc->has_reserved_memory && > + (ms->ram_size < ms->maxram_size)) { > + pc_get_device_memory_range(pcms, &devmem_start, > + &devmem_size); > + if (!pcmc->broken_reserved_end) { > + devmem_start += devmem_size; > + } > + return devmem_start - 1; > + } else { > + return pc_above_4g_end(pcms) - 1; > + } > + } else { > + /* old value for compatibility reasons */ > + return ((hwaddr)1 << cpu->phys_bits) - 1; > + } > } > > + /* 64-bit systems */ > return pc_pci_hole64_start() + pci_hole64_size - 1; > } > > @@ -1867,6 +1894,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > pcmc->pvh_enabled = true; > pcmc->kvmclock_create_always = true; > pcmc->resizable_acpi_blob = true; > + pcmc->fixed_32bit_mem_addr_check = true; > assert(!mc->get_hotplug_handler); > mc->get_hotplug_handler = pc_get_hotplug_handler; > mc->hotplug_allowed = pc_hotplug_allowed; > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 8321f36f97..f100a5de8b 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -517,9 +517,13 @@ DEFINE_I440FX_MACHINE(v8_2, "pc-i440fx-8.2", NULL, > > static void pc_i440fx_8_1_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > + > pc_i440fx_8_2_machine_options(m); > m->alias = NULL; > m->is_default = false; > + pcmc->fixed_32bit_mem_addr_check = false; > + > compat_props_add(m->compat_props, hw_compat_8_1, hw_compat_8_1_len); > compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len); > } > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 0fabece236..5a70d163d0 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -129,6 +129,9 @@ struct PCMachineClass { > > /* resizable acpi blob compat */ > bool resizable_acpi_blob; > + > + /* fixed 32-bit processor address space bound check for memory */ > + bool fixed_32bit_mem_addr_check; > }; > > #define TYPE_PC_MACHINE "generic-pc-machine" > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index d1b80149f2..f8e03dfd46 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -2080,7 +2080,6 @@ int main(int argc, char *argv[]) > test_acpi_piix4_no_acpi_pci_hotplug); > qtest_add_func("acpi/piix4/ipmi", test_acpi_piix4_tcg_ipmi); > qtest_add_func("acpi/piix4/cpuhp", test_acpi_piix4_tcg_cphp); > - qtest_add_func("acpi/piix4/memhp", test_acpi_piix4_tcg_memhp); > qtest_add_func("acpi/piix4/numamem", test_acpi_piix4_tcg_numamem); > qtest_add_func("acpi/piix4/nosmm", test_acpi_piix4_tcg_nosmm); > qtest_add_func("acpi/piix4/smm-compat", > @@ -2088,9 +2087,15 @@ int main(int argc, char *argv[]) > qtest_add_func("acpi/piix4/smm-compat-nosmm", > test_acpi_piix4_tcg_smm_compat_nosmm); > qtest_add_func("acpi/piix4/nohpet", test_acpi_piix4_tcg_nohpet); > - qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm); > - qtest_add_func("acpi/piix4/acpihmat", > - test_acpi_piix4_tcg_acpi_hmat); > + > + /* i386 does not support memory hotplug */ > + if (strcmp(arch, "i386")) { > + qtest_add_func("acpi/piix4/memhp", test_acpi_piix4_tcg_memhp); > + qtest_add_func("acpi/piix4/dimmpxm", > + test_acpi_piix4_tcg_dimm_pxm); > + qtest_add_func("acpi/piix4/acpihmat", > + test_acpi_piix4_tcg_acpi_hmat); > + } > #ifdef CONFIG_POSIX > qtest_add_func("acpi/piix4/acpierst", test_acpi_piix4_acpi_erst); > #endif > @@ -2108,11 +2113,9 @@ int main(int argc, char *argv[]) > test_acpi_q35_tcg_no_acpi_hotplug); > qtest_add_func("acpi/q35/multif-bridge", > test_acpi_q35_multif_bridge); > - qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); > qtest_add_func("acpi/q35/ipmi", test_acpi_q35_tcg_ipmi); > qtest_add_func("acpi/q35/smbus/ipmi", test_acpi_q35_tcg_smbus_ipmi); > qtest_add_func("acpi/q35/cpuhp", test_acpi_q35_tcg_cphp); > - qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp); > qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem); > qtest_add_func("acpi/q35/nosmm", test_acpi_q35_tcg_nosmm); > qtest_add_func("acpi/q35/smm-compat", > @@ -2120,10 +2123,17 @@ int main(int argc, char *argv[]) > qtest_add_func("acpi/q35/smm-compat-nosmm", > test_acpi_q35_tcg_smm_compat_nosmm); > qtest_add_func("acpi/q35/nohpet", test_acpi_q35_tcg_nohpet); > - qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm); > - qtest_add_func("acpi/q35/acpihmat", test_acpi_q35_tcg_acpi_hmat); > qtest_add_func("acpi/q35/acpihmat-noinitiator", > test_acpi_q35_tcg_acpi_hmat_noinitiator); > + > + /* i386 does not support memory hotplug */ > + if (strcmp(arch, "i386")) { > + qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp); > + qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm); > + qtest_add_func("acpi/q35/acpihmat", > + test_acpi_q35_tcg_acpi_hmat); > + qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); > + } > #ifdef CONFIG_POSIX > qtest_add_func("acpi/q35/acpierst", test_acpi_q35_acpi_erst); > #endif > diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c > index c5eb13f349..4f4404a4b1 100644 > --- a/tests/qtest/numa-test.c > +++ b/tests/qtest/numa-test.c > @@ -568,7 +568,7 @@ int main(int argc, char **argv) > qtest_add_data_func("/numa/mon/cpus/partial", args, test_mon_partial); > qtest_add_data_func("/numa/qmp/cpus/query-cpus", args, test_query_cpus); > > - if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) { > + if (!strcmp(arch, "x86_64")) { > qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu); > qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg); > qtest_add_data_func("/numa/pc/hmat/build", args, pc_hmat_build_cfg); > @@ -576,6 +576,11 @@ int main(int argc, char **argv) > qtest_add_data_func("/numa/pc/hmat/erange", args, pc_hmat_erange_cfg); > } > > + if (!strcmp(arch, "i386")) { > + qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu); > + qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg); > + } > + > if (!strcmp(arch, "ppc64")) { > qtest_add_data_func("/numa/spapr/cpu/explicit", args, spapr_numa_cpu); > } > -- > 2.39.1 >
> On 21-Sep-2023, at 1:45 PM, David Hildenbrand <david@redhat.com> wrote: > > On 21.09.23 09:17, Ani Sinha wrote: >> 32-bit systems do not have a reserved memory for hole64 and hotplugging memory >> devices are not supported on those systems. Therefore, the maximum limit of the >> guest physical address in the absence of additional memory devices effectively >> coincides with the end of "above 4G memory space" region. When users configure >> additional memory devices, we need to properly account for the additional device >> memory region so as to find the maximum value of the guest physical address >> and enforce that it is within the physical address space of the processor. For >> 32-bit, this maximum PA will be outside the range of the processor's address >> space. >> With this change, for example, previously this was allowed: >> $ ./qemu-system-x86_64 -cpu pentium -m size=10G >> Now it is no longer allowed: >> $ ./qemu-system-x86_64 -cpu pentium -m size=10G >> qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff phys-bits too low (32) >> For 32-bit, hotplugging additional memory is no longer allowed. > > "32-bit without PAE/PSE36" > >> $ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 >> qemu-system-i386: Address space limit 0xffffffff < 0x1ffffffff phys-bits too low (32) > > We always place the device memory region above 4G. Without PAE/PSE36, you cannot ever possibly make use hotplugged memory, because it would reside > 4g. > > So while the user could have started that QEMU instance, even with an OS that would support memory hotplug, a DIMM above 4G would not have been usable. > > So we're now properly failing for a setup that doesn't make any sense. Good :) > > ... if someone ever cares about making that work, we would have to let the device memory region start below 4g (and obviously, not exceed 4g). > > > So while > > ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 > > fails (because pentium cannot access that memory), what should work is > > ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 -cpu pentium2 > > or > > ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 -cpu pentium,pse36=on > > Because that CPU could actually address that memory somehow (PAE/PSE36). > > > So IMHO, we're now forbidding setups that are impossible. > >> The above is still allowed for older machine types in order to support >> compatibility. Therefore, this still works: >> $ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2 > > Makes sense. (probably nobody cares, but better safe than sorry) > >> After calling CPUID with EAX=0x80000001, all AMD64 compliant processors >> have the longmode-capable-bit turned on in the extended feature flags (bit 29) >> in EDX. The absence of CPUID longmode can be used to differentiate between >> 32-bit and 64-bit processors and is the recommended approach. QEMU takes this >> approach elsewhere (for example, please see x86_cpu_realizefn()) and with >> this change, pc_max_used_gpa() also takes the same approach to detect 32-bit >> processors. >> Unit tests are modified to not run those tests that use memory hotplug >> on 32-bit x86 architecture. > > We could use a different CPU (pentium2) to still run these tests. "pentium2" should work I assume? Yes it does. > [...] > >> @@ -907,12 +907,39 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms) >> static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size) >> { >> X86CPU *cpu = X86_CPU(first_cpu); >> + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); >> + MachineState *ms = MACHINE(pcms); >> + uint64_t devmem_start = 0; >> + ram_addr_t devmem_size = 0; >> - /* 32-bit systems don't have hole64 thus return max CPU address */ >> - if (cpu->phys_bits <= 32) { >> - return ((hwaddr)1 << cpu->phys_bits) - 1; >> + /* >> + * 32-bit systems don't have hole64 but they might have a region for >> + * memory devices. Even if additional hotplugged memory devices might >> + * not be usable by most guest OSes, we need to still consider them for >> + * calculating the highest possible GPA so that we can properly report >> + * if someone configures them on a CPU that cannot possibly address them. >> + */ >> + if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) { >> + /* 32-bit systems */ >> + if (pcmc->fixed_32bit_mem_addr_check) { >> + if (pcmc->has_reserved_memory && >> + (ms->ram_size < ms->maxram_size)) { >> + pc_get_device_memory_range(pcms, &devmem_start, >> + &devmem_size); >> + if (!pcmc->broken_reserved_end) { > > I think you can remove that check. "pcmc->fixed_32bit_mem_addr_check && pcmc->broken_reserved_end" can never hold at the same time. Yeah my bad, I was being too defensive. Will remove. > > broken_reserved_end is only set for QEMU <= 2.4, to work around another broken check. pcmc->fixed_32bit_mem_addr_check is only set for 8.2+. > > Maybe consider calling "fixed_32bit_mem_addr_check" "pcmc->broken_32bit_max_gpa_check" and reverse the logic (treating it like broken_reserved_end). Will fix next version. > > > -- > Cheers, > > David / dhildenb
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 54838c0c41..0aa2f6b6c0 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -907,12 +907,39 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms) static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size) { X86CPU *cpu = X86_CPU(first_cpu); + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); + MachineState *ms = MACHINE(pcms); + uint64_t devmem_start = 0; + ram_addr_t devmem_size = 0; - /* 32-bit systems don't have hole64 thus return max CPU address */ - if (cpu->phys_bits <= 32) { - return ((hwaddr)1 << cpu->phys_bits) - 1; + /* + * 32-bit systems don't have hole64 but they might have a region for + * memory devices. Even if additional hotplugged memory devices might + * not be usable by most guest OSes, we need to still consider them for + * calculating the highest possible GPA so that we can properly report + * if someone configures them on a CPU that cannot possibly address them. + */ + if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) { + /* 32-bit systems */ + if (pcmc->fixed_32bit_mem_addr_check) { + if (pcmc->has_reserved_memory && + (ms->ram_size < ms->maxram_size)) { + pc_get_device_memory_range(pcms, &devmem_start, + &devmem_size); + if (!pcmc->broken_reserved_end) { + devmem_start += devmem_size; + } + return devmem_start - 1; + } else { + return pc_above_4g_end(pcms) - 1; + } + } else { + /* old value for compatibility reasons */ + return ((hwaddr)1 << cpu->phys_bits) - 1; + } } + /* 64-bit systems */ return pc_pci_hole64_start() + pci_hole64_size - 1; } @@ -1867,6 +1894,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) pcmc->pvh_enabled = true; pcmc->kvmclock_create_always = true; pcmc->resizable_acpi_blob = true; + pcmc->fixed_32bit_mem_addr_check = true; assert(!mc->get_hotplug_handler); mc->get_hotplug_handler = pc_get_hotplug_handler; mc->hotplug_allowed = pc_hotplug_allowed; diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 8321f36f97..f100a5de8b 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -517,9 +517,13 @@ DEFINE_I440FX_MACHINE(v8_2, "pc-i440fx-8.2", NULL, static void pc_i440fx_8_1_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); + pc_i440fx_8_2_machine_options(m); m->alias = NULL; m->is_default = false; + pcmc->fixed_32bit_mem_addr_check = false; + compat_props_add(m->compat_props, hw_compat_8_1, hw_compat_8_1_len); compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len); } diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 0fabece236..5a70d163d0 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -129,6 +129,9 @@ struct PCMachineClass { /* resizable acpi blob compat */ bool resizable_acpi_blob; + + /* fixed 32-bit processor address space bound check for memory */ + bool fixed_32bit_mem_addr_check; }; #define TYPE_PC_MACHINE "generic-pc-machine" diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index d1b80149f2..f8e03dfd46 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -2080,7 +2080,6 @@ int main(int argc, char *argv[]) test_acpi_piix4_no_acpi_pci_hotplug); qtest_add_func("acpi/piix4/ipmi", test_acpi_piix4_tcg_ipmi); qtest_add_func("acpi/piix4/cpuhp", test_acpi_piix4_tcg_cphp); - qtest_add_func("acpi/piix4/memhp", test_acpi_piix4_tcg_memhp); qtest_add_func("acpi/piix4/numamem", test_acpi_piix4_tcg_numamem); qtest_add_func("acpi/piix4/nosmm", test_acpi_piix4_tcg_nosmm); qtest_add_func("acpi/piix4/smm-compat", @@ -2088,9 +2087,15 @@ int main(int argc, char *argv[]) qtest_add_func("acpi/piix4/smm-compat-nosmm", test_acpi_piix4_tcg_smm_compat_nosmm); qtest_add_func("acpi/piix4/nohpet", test_acpi_piix4_tcg_nohpet); - qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm); - qtest_add_func("acpi/piix4/acpihmat", - test_acpi_piix4_tcg_acpi_hmat); + + /* i386 does not support memory hotplug */ + if (strcmp(arch, "i386")) { + qtest_add_func("acpi/piix4/memhp", test_acpi_piix4_tcg_memhp); + qtest_add_func("acpi/piix4/dimmpxm", + test_acpi_piix4_tcg_dimm_pxm); + qtest_add_func("acpi/piix4/acpihmat", + test_acpi_piix4_tcg_acpi_hmat); + } #ifdef CONFIG_POSIX qtest_add_func("acpi/piix4/acpierst", test_acpi_piix4_acpi_erst); #endif @@ -2108,11 +2113,9 @@ int main(int argc, char *argv[]) test_acpi_q35_tcg_no_acpi_hotplug); qtest_add_func("acpi/q35/multif-bridge", test_acpi_q35_multif_bridge); - qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); qtest_add_func("acpi/q35/ipmi", test_acpi_q35_tcg_ipmi); qtest_add_func("acpi/q35/smbus/ipmi", test_acpi_q35_tcg_smbus_ipmi); qtest_add_func("acpi/q35/cpuhp", test_acpi_q35_tcg_cphp); - qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp); qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem); qtest_add_func("acpi/q35/nosmm", test_acpi_q35_tcg_nosmm); qtest_add_func("acpi/q35/smm-compat", @@ -2120,10 +2123,17 @@ int main(int argc, char *argv[]) qtest_add_func("acpi/q35/smm-compat-nosmm", test_acpi_q35_tcg_smm_compat_nosmm); qtest_add_func("acpi/q35/nohpet", test_acpi_q35_tcg_nohpet); - qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm); - qtest_add_func("acpi/q35/acpihmat", test_acpi_q35_tcg_acpi_hmat); qtest_add_func("acpi/q35/acpihmat-noinitiator", test_acpi_q35_tcg_acpi_hmat_noinitiator); + + /* i386 does not support memory hotplug */ + if (strcmp(arch, "i386")) { + qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp); + qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm); + qtest_add_func("acpi/q35/acpihmat", + test_acpi_q35_tcg_acpi_hmat); + qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); + } #ifdef CONFIG_POSIX qtest_add_func("acpi/q35/acpierst", test_acpi_q35_acpi_erst); #endif diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c index c5eb13f349..4f4404a4b1 100644 --- a/tests/qtest/numa-test.c +++ b/tests/qtest/numa-test.c @@ -568,7 +568,7 @@ int main(int argc, char **argv) qtest_add_data_func("/numa/mon/cpus/partial", args, test_mon_partial); qtest_add_data_func("/numa/qmp/cpus/query-cpus", args, test_query_cpus); - if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) { + if (!strcmp(arch, "x86_64")) { qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu); qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg); qtest_add_data_func("/numa/pc/hmat/build", args, pc_hmat_build_cfg); @@ -576,6 +576,11 @@ int main(int argc, char **argv) qtest_add_data_func("/numa/pc/hmat/erange", args, pc_hmat_erange_cfg); } + if (!strcmp(arch, "i386")) { + qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu); + qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg); + } + if (!strcmp(arch, "ppc64")) { qtest_add_data_func("/numa/spapr/cpu/explicit", args, spapr_numa_cpu); }
32-bit systems do not have a reserved memory for hole64 and hotplugging memory devices are not supported on those systems. Therefore, the maximum limit of the guest physical address in the absence of additional memory devices effectively coincides with the end of "above 4G memory space" region. When users configure additional memory devices, we need to properly account for the additional device memory region so as to find the maximum value of the guest physical address and enforce that it is within the physical address space of the processor. For 32-bit, this maximum PA will be outside the range of the processor's address space. With this change, for example, previously this was allowed: $ ./qemu-system-x86_64 -cpu pentium -m size=10G Now it is no longer allowed: $ ./qemu-system-x86_64 -cpu pentium -m size=10G qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff phys-bits too low (32) For 32-bit, hotplugging additional memory is no longer allowed. $ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2 qemu-system-i386: Address space limit 0xffffffff < 0x1ffffffff phys-bits too low (32) The above is still allowed for older machine types in order to support compatibility. Therefore, this still works: $ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2 After calling CPUID with EAX=0x80000001, all AMD64 compliant processors have the longmode-capable-bit turned on in the extended feature flags (bit 29) in EDX. The absence of CPUID longmode can be used to differentiate between 32-bit and 64-bit processors and is the recommended approach. QEMU takes this approach elsewhere (for example, please see x86_cpu_realizefn()) and with this change, pc_max_used_gpa() also takes the same approach to detect 32-bit processors. Unit tests are modified to not run those tests that use memory hotplug on 32-bit x86 architecture. Finally, a new compatibility flag is introduced to retain the old behavior for pc_max_used_gpa() for machines 8.1 and older. Suggested-by: David Hildenbrand <david@redhat.com> Signed-off-by: Ani Sinha <anisinha@redhat.com> --- hw/i386/pc.c | 34 +++++++++++++++++++++++++++++++--- hw/i386/pc_piix.c | 4 ++++ include/hw/i386/pc.h | 3 +++ tests/qtest/bios-tables-test.c | 26 ++++++++++++++++++-------- tests/qtest/numa-test.c | 7 ++++++- 5 files changed, 62 insertions(+), 12 deletions(-) changelog: v3: still accounting for additional memory device region above 4G. unit tests fixed (not running for 32-bit where mem hotplug is used). v2: removed memory hotplug region from max_gpa. added compat knobs.