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

[Ani Sinha]

> On 22-Sep-2023, at 4:12 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> On 22/9/23 06:16, Ani Sinha wrote:
>> 32-bit x86 systems do not have a reserved memory for hole64. On those 32-bit
>> systems without PSE36 or PAE CPU features, hotplugging memory devices are not
>> supported by QEMU as QEMU always places hotplugged memory above 4 GiB 
>> boundary
>> which is beyond the physical address space of the processor. Linux guests 
>> also
>> does not support memory hotplug on those systems. Please see Linux
>> kernel commit b59d02ed08690 ("mm/memory_hotplug: disable the functionality
>> for 32b") for more details.
>> 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 for 32-bit x86 without PAE/PSE36. When users
>> configure additional memory devices, after properly accounting for the
>> additional device memory region to find the maximum value of the guest
>> physical address, the address will be outside the range of the processor's
>> physical address space.
>> This change adds improvements to take above into consideration.
>> For example, previously this was allowed:
>> $ ./qemu-system-x86_64 -cpu pentium -m size=10G
>> With this change 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)
>> However, the following are allowed since on both cases physical address
>> space of the processor is 36 bits:
>> $ ./qemu-system-x86_64 -cpu pentium2 -m size=10G
>> $ ./qemu-system-x86_64 -cpu pentium,pse36=on -m size=10G
>> For 32-bit, without PAE/PSE36, 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)
>> $ ./qemu-system-i386 -machine q35 -m size=1G,maxmem=3G,slots=2
>> qemu-system-i386: Address space limit 0xffffffff < 0x1ffffffff phys-bits too 
>> low (32)
>> A new compatibility flag is introduced to make sure pc_max_used_gpa() keeps
>> returning the old value for machines 8.1 and older.
>> Therefore, the above is still allowed for older machine types in order to 
>> support
>> compatibility. Hence, the following still works:
>> $ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2
>> $ ./qemu-system-i386 -machine pc-q35-8.1 -m size=1G,maxmem=3G,slots=2
>> Further, following is also allowed as with PSE36, the processor has 36-bit
>> address space:
>> $ ./qemu-system-i386 -cpu 486,pse36=on -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()), With
>> this change, pc_max_used_gpa() also uses the same method to detect 32-bit
>> processors.
>> Unit tests are modified to not run 32-bit x86 tests that use memory hotplug.
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>>  hw/i386/pc.c                   | 31 ++++++++++++++++++++++++++++---
>>  hw/i386/pc_piix.c              |  4 ++++
>>  hw/i386/pc_q35.c               |  2 ++
>>  include/hw/i386/pc.h           |  6 ++++++
>>  tests/qtest/bios-tables-test.c | 26 ++++++++++++++++++--------
>>  tests/qtest/numa-test.c        |  7 ++++++-
>>  6 files changed, 64 insertions(+), 12 deletions(-)
> 
> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 54838c0c41..2a689cf0bd 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -907,12 +907,37 @@ 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->broken_32bit_mem_addr_check) {
> 
> Nitpicking, code is simplified if you invert this condition check.

Maybe it reads better, but simplified? .. I am not so sure.

> 
>> +            if (pcmc->has_reserved_memory &&
>> +                (ms->ram_size < ms->maxram_size)) {
>> +                pc_get_device_memory_range(pcms, &devmem_start,
>> +                                           &devmem_size);
>> +                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;
> 
> Since you change this line, can we convert to
> MAKE_64BIT_MASK(0, cpu->phys_bits) ?

Doesn’t the existing code reads better? Assuming that the macro does exactly 
the same thing, one has to still look up the definition. And 

 (((~0ULL) >> (64 - (length))) << (shift))

Is such a brain twister :-) 

> 
>> +        }
>>      }
>>  +    /* 64-bit systems */
>>      return pc_pci_hole64_start() + pci_hole64_size - 1;
>>  }