On Tue, Nov 14, 2017 at 09:15:52AM +0800, zhuyi...@huawei.com wrote:
> From: Zhu Yijun <zhuyi...@huawei.com>
>
> 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 <zhuyi...@huawei.com>
> ---
> 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("ram@%" 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
