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

[Ani Sinha <anisinha@redhat.com>]

> 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.