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

[David Hildenbrand]

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)

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