Re: [Qemu-arm] [Qemu-devel] [RFC 3/5] hw/arm: add scattered RAM memory region support

[Andrew Jones]

On Tue, Nov 14, 2017 at 09:15:52AM +0800, address@hidden wrote:
> From: Zhu Yijun <address@hidden>
> 
> Dig out reserved memory holes and collect scattered RAM memory
> regions by adding mem_list member in arm_boot_info struct.
> 
> Signed-off-by: Zhu Yijun <address@hidden>
> ---
>  hw/arm/boot.c        |   8 ++++
>  hw/arm/virt.c        | 101 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/hw/arm/arm.h |   1 +
>  3 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index c2720c8..30438f4 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -836,6 +836,14 @@ static void arm_load_kernel_notify(Notifier *notifier, 
> void *data)
>       */
>      assert(!(info->secure_board_setup && kvm_enabled()));
>  
> +    /* If machine is not virt, the mem_list will empty. */
> +    if (QLIST_EMPTY(&vms->bootinfo.mem_list)) {
> +        RAMRegion *new = g_new(RAMRegion, 1);
> +        new->base = info->loader_start;
> +        new->size = info->ram_size;
> +        QLIST_INSERT_HEAD(&vms->bootinfo.mem_list, new, next);
> +    }
> +
>      info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
>  
>      /* Load the kernel.  */
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ddde5e1..ff334c1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -56,6 +56,7 @@
>  #include "hw/smbios/smbios.h"
>  #include "qapi/visitor.h"
>  #include "standard-headers/linux/input.h"
> +#include "hw/vfio/vfio-common.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1225,6 +1226,98 @@ void virt_machine_done(Notifier *notifier, void *data)
>      virt_build_smbios(vms);
>  }
>  
> +static void handle_reserved_ram_region_overlap(void)
> +{
> +    hwaddr cur_end, next_end;
> +    RAMRegion *reg, *next_reg, *tmp_reg;
> +
> +    QLIST_FOREACH(reg, &reserved_ram_regions, next) {
> +        next_reg = QLIST_NEXT(reg, next);
> +
> +        while (next_reg && next_reg->base <= (reg->base + reg->size)) {
> +            next_end = next_reg->base + next_reg->size;
> +            cur_end = reg->base + reg->size;
> +            if (next_end > cur_end) {
> +                reg->size += (next_end - cur_end);
> +            }
> +
> +            tmp_reg = QLIST_NEXT(next_reg, next);
> +            QLIST_REMOVE(next_reg, next);
> +            g_free(next_reg);
> +            next_reg = tmp_reg;
> +        }
> +    }
> +}

Why isn't the above integrated into the reserved ram region insertion?

> +
> +static void update_memory_regions(VirtMachineState *vms, hwaddr ram_size)
> +{
> +
> +    RAMRegion *new, *reg, *last = NULL;
> +    hwaddr virt_start, virt_end;

Either need a blank line here, or to initialize virt_start/end on the
declaration lines.

> +    virt_start = vms->memmap[VIRT_MEM].base;
> +    virt_end = virt_start + ram_size - 1;
> +
> +    handle_reserved_ram_region_overlap();
> +
> +    QLIST_FOREACH(reg, &reserved_ram_regions, next) {
> +        if (reg->base >= virt_start && reg->base < virt_end) {

What about the case where reg->base + reg->size > virt_start?

> +            if (reg->base == virt_start) {
> +                virt_start += reg->size;

We can't move virt_start without breaking AAVMF.

> +                virt_end += reg->size;

We can't increase virt_end without checking it against RAMLIMIT.

> +                continue;
> +            } else {
> +                new = g_new(RAMRegion, 1);
> +                new->base = virt_start;
> +                new->size = reg->base - virt_start;
> +                virt_start = reg->base + reg->size;
> +            }
> +
> +            if (QLIST_EMPTY(&vms->bootinfo.mem_list)) {
> +                QLIST_INSERT_HEAD(&vms->bootinfo.mem_list, new, next);
> +            } else {
> +                QLIST_INSERT_AFTER(last, new, next);
> +            }
> +
> +            last = new;
> +            ram_size -= new->size;
> +            virt_end += reg->size;
> +        }
> +    }
> +
> +    if (ram_size > 0) {
> +        new = g_new(RAMRegion, 1);
> +        new->base = virt_start;
> +        new->size = ram_size;
> +
> +        if (QLIST_EMPTY(&vms->bootinfo.mem_list)) {
> +            QLIST_INSERT_HEAD(&vms->bootinfo.mem_list, new, next);
> +        } else {
> +            QLIST_INSERT_AFTER(last, new, next);
> +        }
> +    }

Where's the else? ram_size <= 0 is not likely something we should ignore.

> +}
> +
> +static void create_ram_alias(VirtMachineState *vms,
> +                             MemoryRegion *sysmem,
> +                             MemoryRegion *ram)
> +{
> +    RAMRegion *reg;
> +    MemoryRegion *ram_memory;
> +    char *nodename;
> +    hwaddr sz = 0;
> +
> +    QLIST_FOREACH(reg, &vms->bootinfo.mem_list, next) {
> +        nodename = g_strdup_printf("address@hidden" PRIx64, reg->base);
> +        ram_memory = g_new(MemoryRegion, 1);
> +        memory_region_init_alias(ram_memory, NULL, nodename, ram, sz,
> +                                 reg->size);
> +        memory_region_add_subregion(sysmem, reg->base, ram_memory);
> +        sz += reg->size;
> +
> +        g_free(nodename);
> +    }
> +}

Instead of using memory region aliases, it would be best if each RAM
region was modeled with pc-dimms, as that would move us towards supporting
memory hotplug and allow the regions to be explicitly identified
(start/size) on the command line - supporting migration. Actually, how
does this series address migration? What if the host we migrate to doesn't
have the same reserved regions in sysfs?

> +
>  static void virt_ram_memory_region_init(Notifier *notifier, void *data)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
> @@ -1232,10 +1325,15 @@ static void virt_ram_memory_region_init(Notifier 
> *notifier, void *data)
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      VirtMachineState *vms = container_of(notifier, VirtMachineState,
>                                           ram_memory_region_init);
> +    RAMRegion *first_mem_reg;
>  
>      memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram",
>                                           machine->ram_size);
> -    memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram);
> +    update_memory_regions(vms, machine->ram_size);
> +    create_ram_alias(vms, sysmem, ram);
> +
> +    first_mem_reg = QLIST_FIRST(&vms->bootinfo.mem_list);
> +    vms->bootinfo.loader_start = first_mem_reg->base;
>  }
>  
>  static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
> @@ -1458,7 +1556,6 @@ static void machvirt_init(MachineState *machine)
>      vms->bootinfo.initrd_filename = machine->initrd_filename;
>      vms->bootinfo.nb_cpus = smp_cpus;
>      vms->bootinfo.board_id = -1;
> -    vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base;
>      vms->bootinfo.get_dtb = machvirt_dtb;
>      vms->bootinfo.firmware_loaded = firmware_loaded;
>      arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo);
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index ce769bd..d953026 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -124,6 +124,7 @@ struct arm_boot_info {
>      bool secure_board_setup;
>  
>      arm_endianness endianness;
> +    QLIST_HEAD(, RAMRegion) mem_list;
>  };
>  
>  /**
> -- 
> 1.8.3.1
> 
> 
>

Thanks,
drew