> On 08-Sep-2023, at 3:58 PM, David Hildenbrand <da...@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: imamm...@redhat.com
>> Signed-off-by: Ani Sinha <anisi...@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
