[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] hw/i386/pc: improve physical address space bound check fo
|
From: |
Ani Sinha |
|
Subject: |
Re: [PATCH v3] hw/i386/pc: improve physical address space bound check for 32-bit systems |
|
Date: |
Thu, 21 Sep 2023 13:23:04 +0530 |
> 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
>