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

[Ani Sinha]

> 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