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

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

Posted by Ani Sinha 7 months, 2 weeks ago

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.

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

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

Posted by Ani Sinha 7 months, 2 weeks ago

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
>

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

Posted by David Hildenbrand 7 months, 2 weeks ago

On 19.09.23 13:52, Ani Sinha wrote:
> 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;
>> +        }

I think you should use the logic from v1.

My comment regarding memory hotplug was primarily about (Linux) guest 
support.

We should not optimize for 32bit processors (e.g., try placing the 
device memory region below 4g), but you can still consider the region 
and properly account it.

-- 
Cheers,

David / dhildenb

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

Posted by Ani Sinha 7 months, 2 weeks ago

On Tue, Sep 19, 2023 at 6:10 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.09.23 13:52, Ani Sinha wrote:
> > 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;
> >> +        }
>
> I think you should use the logic from v1.
>
> My comment regarding memory hotplug was primarily about (Linux) guest
> support.
>
> We should not optimize for 32bit processors (e.g., try placing the
> device memory region below 4g), but you can still consider the region
> and properly account it.

I am confused. So maybe you can send a patch.