Re: [PATCH v2] hw/i386/pc: improve physical address space bound check for 32-bit systems

[Ani Sinha <anisinha@redhat.com>]

On Tue, Sep 19, 2023 at 4:08 PM Ani Sinha <anisinha@redhat.com> wrote:
>
> 32-bit systems do not have a reserved memory for hole64 and memory hotplug is
> not supported on those systems. Therefore, the maximum limit of the guest
> physical address coincides with the end of "above 4G memory space" region.
> Make sure that the end of "above 4G memory" is still addressible by the
> guest processor with its available address bits. 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)
>
> 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.
>
> Finally, a new compatibility flag is introduced to retain the old behavior
> for pc_max_used_gpa() for macines 8.1 and older.
typo - will fix in v3                   ^^^^^^^^

btw, does this patch break it for processors < 32-bit? For them clearly

x86ms->above_4g_mem_start = 4 * GiB;

does not work.


>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  hw/i386/pc.c         | 17 ++++++++++++++---
>  hw/i386/pc_piix.c    |  4 ++++
>  include/hw/i386/pc.h |  3 +++
>  3 files changed, 21 insertions(+), 3 deletions(-)
>
> changelog:
> 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..fea97ee258 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -907,10 +907,20 @@ 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);
>
> -    /* 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 and does not support
> +     * memory hotplug.
> +     */
> +    if (pcmc->fixed_32bit_mem_addr_check) {
> +        if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) {
> +            return pc_above_4g_end(pcms) - 1;
> +        }
> +    } else {
> +        if (cpu->phys_bits <= 32) {
> +            return ((hwaddr)1 << cpu->phys_bits) - 1;
> +        }
>      }
>
>      return pc_pci_hole64_start() + pci_hole64_size - 1;
> @@ -1867,6 +1877,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"
> --
> 2.39.1
>