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

[Ani Sinha]

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