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

[Ani Sinha <anisinha@redhat.com>]

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

$ ./qemu-system-x86_64 -cpu pentium -m size=10G, -monitor stdio -qmp tcp:0:5555,server,nowait
QEMU 8.1.50 monitor - type 'help' for more information
VNC server running on ::1:5900
(qemu) 



> 
> Why is that not sufficient or why can't that be extended?
> 
>> +
>>      /*
>>       * Split single memory region and use aliases to address portions of it,
>>       * done for backwards compatibility with older qemus.
>> @@ -1845,6 +1857,38 @@ static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp)
>>      return true;
>>  }
>>  +static bool pc_mem_hotplug_allowed(MachineState *ms,
>> +                                   MemoryRegion *mr, Error **errp)
>> +{
>> +    hwaddr maxphysaddr;
>> +    uint64_t dimm_size, size, ram_size, total_mem_size;
>> +    X86CPU *cpu = X86_CPU(first_cpu);
>> +
>> +    if (!mr) {
>> +        return true;
>> +    }
>> +
>> +    dimm_size = ms->device_memory->dimm_size;
>> +    size = memory_region_size(mr);
>> +    ram_size = (uint64_t) ms->ram_size;
>> +    total_mem_size = ram_size + dimm_size + size;
> 
> That's wrong. The sizes does not tell you where the devices are actually located in the address space.
> 
>> +
>> +    maxphysaddr = ((hwaddr)1 << cpu->phys_bits) - 1;
>> +
>> +    /*
>> +     * total memory after hotplug will exceed the maximum physical
>> +     * address limit of the processor. So hotplug cannot be allowed.
>> +     */
>> +    if ((total_mem_size > (uint64_t)maxphysaddr + 1) &&
>> +        (total_mem_size > ram_size + dimm_size)) {
>> +        error_setg(errp, "Address space limit 0x%"PRIx64" < 0x%"PRIx64
>> +                   " phys-bits too low (%u)",
>> +                   maxphysaddr, total_mem_size, cpu->phys_bits);
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>>  static void pc_machine_class_init(ObjectClass *oc, void *data)
>>  {
>>      MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -1870,6 +1914,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      assert(!mc->get_hotplug_handler);
>>      mc->get_hotplug_handler = pc_get_hotplug_handler;
>>      mc->hotplug_allowed = pc_hotplug_allowed;
>> +    mc->mem_hotplug_allowed = pc_mem_hotplug_allowed;
>>      mc->cpu_index_to_instance_props = x86_cpu_index_to_props;
>>      mc->get_default_cpu_node_id = x86_get_default_cpu_node_id;
>>      mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids;
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index 667d56bd29..825bc593ae 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -57,6 +57,7 @@ static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
>>  {
>>      const uint64_t used_region_size = ms->device_memory->used_region_size;
>>      const uint64_t size = memory_region_size(mr);
>> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>>        /* we will need a new memory slot for kvm and vhost */
>>      if (kvm_enabled() && !kvm_has_free_slot(ms)) {
>> @@ -68,6 +69,11 @@ static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
>>          return;
>>      }
>>  +    if (mc->mem_hotplug_allowed &&
>> +        (!(mc->mem_hotplug_allowed(ms, mr, errp)))) {
>> +        return;
>> +    }
>> +
>>      /* will we exceed the total amount of memory specified */
>>      if (used_region_size + size < used_region_size ||
>>          used_region_size + size > ms->maxram_size - ms->ram_size) {
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 3b541ffd24..84b199ee51 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -210,6 +210,13 @@ typedef struct {
>>   *    false is returned, an error must be set to show the reason of
>>   *    the rejection.  If the hook is not provided, all hotplug will be
>>   *    allowed.
>> + * @mem_hotplug_allowed:
>> + *    If the hook is provided, then it'll be called for each memory device
>> + *    hotplug to check whether the mem device hotplug is allowed.  Return
>> + *    true to grant allowance or false to reject the hotplug.  When
>> + *    false is returned, an error must be set to show the reason of
>> + *    the rejection.  If the hook is not provided, all mem hotplug will be
>> + *    allowed.
> 
> That's nasty.
> 
> 1) The machine hotplug handler already is in charge of plugging such devices. It could perform such checks there but,
> 
> 2) Why even allow the device memory region to exceed maxphysaddr?
> 
> 
> Instead, we should probably fail creating the device managed region if it would end up exceeding maxphysaddr.
> 
> pc_memory_init()-> ... -> machine_memory_devices_init()
> 
> Can't we make sure in pc_memory_init() that we can never have memory devices being plugged into inaccessible regions? Or check back later once we know the limit (if not already known)?
> 
> -- 
> Cheers,
> 
> David / dhildenb