Re: [PATCH] mem/x86: add processor address space check for VM memory

[David Hildenbrand <david@redhat.com>]

On 08.09.23 18:02, David Hildenbrand wrote:
> On 08.09.23 17:13, Ani Sinha wrote:
>>
>>
>>> On 08-Sep-2023, at 7:46 PM, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 08.09.23 16:12, Ani Sinha wrote:
>>>>> On 08-Sep-2023, at 3:58 PM, David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> On 08.09.23 11:50, Ani Sinha wrote:
>>>>>> Depending on the number of available address bits of the current processor, a
>>>>>> VM can only use a certain maximum amount of memory and no more. This change
>>>>>> makes sure that a VM is not configured to have more memory than what it can use
>>>>>> with the current processor settings when started. Additionally, the change adds
>>>>>> checks during memory hotplug to ensure that the VM does not end up getting more
>>>>>> memory than what it can actually use after hotplug.
>>>>>> Currently, both the above checks are only for pc (x86) platform.
>>>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1235403
>>>>>> CC: imammedo@redhat.com
>>>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>>>> ---
>>>>>>    hw/i386/pc.c           | 45 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>    hw/mem/memory-device.c |  6 ++++++
>>>>>>    include/hw/boards.h    |  9 +++++++++
>>>>>>    3 files changed, 60 insertions(+)
>>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>>> index 54838c0c41..f84e4c4916 100644
>>>>>> --- a/hw/i386/pc.c
>>>>>> +++ b/hw/i386/pc.c
>>>>>> @@ -31,6 +31,7 @@
>>>>>>    #include "hw/i386/topology.h"
>>>>>>    #include "hw/i386/fw_cfg.h"
>>>>>>    #include "hw/i386/vmport.h"
>>>>>> +#include "hw/mem/memory-device.h"
>>>>>>    #include "sysemu/cpus.h"
>>>>>>    #include "hw/block/fdc.h"
>>>>>>    #include "hw/ide/internal.h"
>>>>>> @@ -1006,6 +1007,17 @@ void pc_memory_init(PCMachineState *pcms,
>>>>>>            exit(EXIT_FAILURE);
>>>>>>        }
>>>>>>    +    /*
>>>>>> +     * check if the VM started with more ram configured than max physical
>>>>>> +     * address available with the current processor.
>>>>>> +     */
>>>>>> +    if (machine->ram_size > maxphysaddr + 1) {
>>>>>> +        error_report("Address space limit 0x%"PRIx64" < 0x%"PRIx64
>>>>>> +                     " (max configured memory), phys-bits too low (%u)",
>>>>>> +                     maxphysaddr, machine->ram_size, cpu->phys_bits);
>>>>>> +        exit(EXIT_FAILURE);
>>>>>> +    }
>>>>>
>>>>> ... I know that this used to be a problem in the past, but nowadays we already do have similar checks in place?
>>>>>
>>>>> $ ./build/qemu-system-x86_64 -m 4T -machine q35,memory-backend=mem0 -object memory-backend-ram,id=mem0,size=4T,reserve=off
>>>>> qemu-system-x86_64: Address space limit 0xffffffffff < 0x5077fffffff phys-bits too low (40)
>>>> So you are saying that this is OK and should be allowed? On a 32 bit processor that can access only 4G memory, I am spinning up a 10G VM.
>>>
>>> Would that 32bit process have PAE (Physical Address Extension) and still be able to access that memory?
>>
>>
>> You are sidestepping my point. Sure, we can improve the condition check by checking for PAE CPUID etc but that is not the issue I am trying too point out. What if the processor did not have PAE? Would we allow a VM to have memory size which the processor can’t access? There is no such check today it would seem.
>>
> 
> Indeed, because the implementation for 32bit in pc_max_used_gpa() is wrong.
> 
> Note that for 64bit it does the right thing, even with memory hotplug,
> because the PCI64 hole is placed above the memory device region.
> 
> So I think we should tackle that via pc_max_used_gpa().
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 54838c0c41..d187890675 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -908,9 +908,12 @@ static hwaddr pc_max_used_gpa(PCMachineState *pcms,
> uint64_t pci_hole64_size)
>    {
>        X86CPU *cpu = X86_CPU(first_cpu);
> 
> -    /* 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 we might have a region for
> +     * memory hotplug.
> +     */
> +    if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) {
> +        return pc_pci_hole64_start() - 1;
>        }
> 
>        return pc_pci_hole64_start() + pci_hole64_size - 1;
> 
> 
> That implies:
> 
> ./build/qemu-system-x86_64 -cpu pentium -m size=4G -nodefaults -nographic
> qemu-system-x86_64: Address space limit 0xffffffff < 0x13fffffff
> phys-bits too low (32)
> 
> As we have memory over 4G (due to PCI hole), that would now correctly fail.
> 
> However, what works is:
> 
> ./build/qemu-system-x86_64 -cpu pentium -m size=3G -nodefaults -nographic
> 
> 
> Weirdly enough, when setting cpu->phys_bits, we take care of PSE36 and
> allow for 36bits in the address space.
> 
> 
> So what works:
> 
> ./build/qemu-system-x86_64 -cpu pentium,pse36=on -m size=32G -nodefaults
> -nographic
> 
> And what doesn't:
> 
>    ./build/qemu-system-x86_64 -cpu pentium,pse36=on -m size=64G
> -nodefaults -nographic -S
> qemu-system-x86_64: Address space limit 0xfffffffff < 0x103fffffff
> phys-bits too low (36)
> 
> 
> However, we don't seem to have such handling in place for PAE (do we
> have to extend that handling in x86_cpu_realizefn()?). Maybe pae should
> always imply pse36, not sure ...
> 

Reading trustworthy wikipedia:

"Physical Address Extension (PAE) is an alternative to PSE-36 which also 
allows 36-bit addressing."

So maybe we have to consider PAE as well.

-- 
Cheers,

David / dhildenb