Re: [Qemu-devel] [RFC 4/5] hw/arm/boot: set fdt size cell of memory node from mem_list

[Zhu Yijun]

On 2017/11/14 22:51, Andrew Jones wrote:
> On Tue, Nov 14, 2017 at 09:15:53AM +0800, address@hidden wrote:
>> From: Zhu Yijun <address@hidden>
>>
>> In case of accessing memory holes, we create memory fdt node by
>> traversing mem_list and adjust memory address if the addr is not
>> belong to RAM memory region.
>>
>> Signed-off-by: Zhu Yijun <address@hidden>
>> ---
>>  hw/arm/boot.c | 151 
>> ++++++++++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 109 insertions(+), 42 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 30438f4..f4ca599 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -384,6 +384,25 @@ static void set_kernel_args_old(const struct 
>> arm_boot_info *info)
>>      }
>>  }
>>  
>> +static char *create_memory_fdt(void *fdt, uint32_t acells, hwaddr mem_base,
>> +                                          uint32_t scells, hwaddr mem_len)
>> +{
>> +    char *nodename = NULL;
>> +    int rc;
>> +
>> +    nodename = g_strdup_printf("/address@hidden" PRIx64, mem_base);
>> +    qemu_fdt_add_subnode(fdt, nodename);
>> +    qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
>> +    rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells, 
>> mem_base,
>> +                                      scells, mem_len);
>> +    if (rc < 0) {
>> +        fprintf(stderr, "couldn't set %s/reg\n", nodename);
>> +        g_free(nodename);
>> +    }
>> +
>> +    return nodename;
>> +}
>> +
>>  /**
>>   * load_dtb() - load a device tree binary image into memory
>>   * @addr:       the address to load the image at
>> @@ -412,7 +431,7 @@ static int load_dtb(hwaddr addr, const struct 
>> arm_boot_info *binfo,
>>      uint32_t acells, scells;
>>      char *nodename;
>>      unsigned int i;
>> -    hwaddr mem_base, mem_len;
>> +    RAMRegion *reg;
>>  
>>      if (binfo->dtb_filename) {
>>          char *filename;
>> @@ -464,49 +483,65 @@ static int load_dtb(hwaddr addr, const struct 
>> arm_boot_info *binfo,
>>          goto fail;
>>      }
>>  
>> +    /*
>> +     * Turn the /memory node created before into a NOP node, then create
>> +     * /address@hidden nodes for all numa nodes respectively.
>> +     */
>> +    qemu_fdt_nop_node(fdt, "/memory");
>> +
>>      if (nb_numa_nodes > 0) {
>> -        /*
>> -         * Turn the /memory node created before into a NOP node, then create
>> -         * /address@hidden nodes for all numa nodes respectively.
>> -         */
>> -        qemu_fdt_nop_node(fdt, "/memory");
>> -        mem_base = binfo->loader_start;
>> +        hwaddr reg_offset = 0; /* region base addr offset */
>> +        hwaddr reg_alloc_size = 0; /* region consumed size */
>> +        hwaddr node_mem_size = 0;
>> +        RAMRegion *tmp_reg = QLIST_FIRST(&binfo->mem_list);
>> +
>>          for (i = 0; i < nb_numa_nodes; i++) {
>> -            mem_len = numa_info[i].node_mem;
>> -            nodename = g_strdup_printf("/address@hidden" PRIx64, mem_base);
>> -            qemu_fdt_add_subnode(fdt, nodename);
>> -            qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
>> -            rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
>> -                                              acells, mem_base,
>> -                                              scells, mem_len);
>> -            if (rc < 0) {
>> -                fprintf(stderr, "couldn't set %s/reg for node %d\n", 
>> nodename,
>> -                        i);
>> -                goto fail;
>> -            }
>> +            node_mem_size = numa_info[i].node_mem;
>> +            QLIST_FOREACH(reg, &binfo->mem_list, next) {
>> +                if (reg->base != tmp_reg->base) {
>> +                    continue;
>> +                }
>>  
>> -            qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
>> -            mem_base += mem_len;
>> -            g_free(nodename);
>> -        }
>> -    } else {
>> -        Error *err = NULL;
>> +                if (node_mem_size >= (reg->size - reg_offset)) {
>> +                    reg_alloc_size = reg->size - reg_offset;
>> +                } else {
>> +                    reg_alloc_size = node_mem_size;
>> +                }
>>  
>> -        rc = fdt_path_offset(fdt, "/memory");
>> -        if (rc < 0) {
>> -            qemu_fdt_add_subnode(fdt, "/memory");
>> -        }
>> +                nodename = create_memory_fdt(fdt, acells,
>> +                                             reg->base + reg_offset,
>> +                                             scells, reg_alloc_size);
>> +                if (!nodename) {
>> +                    goto fail;
>> +                }
>> +
>> +                qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
>> +                g_free(nodename);
>>  
>> -        if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) {
>> -            qemu_fdt_setprop_string(fdt, "/memory", "device_type", 
>> "memory");
>> +                node_mem_size -= reg_alloc_size;
>> +                reg_offset += reg_alloc_size;
>> +                tmp_reg = reg;
>> +
>> +                /* The region is depleted */
>> +                if (reg->size == reg_offset) {
>> +                    reg_offset = 0;
>> +                    tmp_reg = QLIST_NEXT(reg, next);
>> +                }
>> +
>> +                if (node_mem_size == 0) {
>> +                    break;
>> +                }
>> +            }
>>          }
>> +    } else {
>> +        QLIST_FOREACH(reg, &binfo->mem_list, next) {
>> +            nodename = create_memory_fdt(fdt, acells, reg->base,
>> +                                         scells, reg->size);
>> +            if (!nodename) {
>> +                goto fail;
>> +            }
>>  
>> -        rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
>> -                                          acells, binfo->loader_start,
>> -                                          scells, binfo->ram_size);
>> -        if (rc < 0) {
>> -            fprintf(stderr, "couldn't set /memory/reg\n");
>> -            goto fail;
>> +            g_free(nodename);
>>          }
>>      }
> I think we should move the numa allocation into board specific code,
> mach-virt, and we should also map dimms to numa nodes, rather than
> just dividing memory up.

OK, I will reconsider it.
>>  
>> @@ -814,6 +849,35 @@ static uint64_t load_aarch64_image(const char 
>> *filename, hwaddr mem_base,
>>      return size;
>>  }
>>  
>> +/* Find the max size memory region after info->initrd_start.
>> + * TODO: we may have a corner case where the memory node size may not 
>> correctly
>> + * fit into kernel/initrd/dtb Image sizes.
>> + * */
>> +static RAMRegion *find_initrd_memregion(const struct arm_boot_info *info,
>> +                                        hwaddr *initrd_start)
>> +{
>> +    RAMRegion *reg, *initrd_reg = NULL;
>> +    hwaddr max_size = 0;
>> +    hwaddr addr = *initrd_start;
>> +
>> +    QLIST_FOREACH(reg, &info->mem_list, next) {
>> +        if (addr < reg->base) {
>> +            if (max_size < reg->size) {
>> +                max_size = reg->size;
>> +                *initrd_start = reg->base;
>> +                initrd_reg = reg;
>> +            }
>> +        } else if (addr >= reg->base && addr < (reg->base + reg->size)) {
>> +            if (max_size < reg->base + reg->size - addr) {
>> +                max_size = reg->base + reg->size - addr;
>> +                initrd_reg = reg;
>> +            }
>> +        }
>> +    }
>> +
>> +    return initrd_reg;
>> +}
>> +
>>  static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>  {
>>      CPUState *cs;
>> @@ -837,11 +901,11 @@ 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)) {
>> +    if (QLIST_EMPTY(&info->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);
>> +        QLIST_INSERT_HEAD(&info->mem_list, new, next);
>>      }
>>  
>>      info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
>> @@ -973,14 +1037,16 @@ static void arm_load_kernel_notify(Notifier 
>> *notifier, void *data)
>>          uint32_t fixupcontext[FIXUP_MAX];
>>  
>>          if (info->initrd_filename) {
>> +            RAMRegion *reg = find_initrd_memregion(info, 
>> &info->initrd_start);
>> +
>>              initrd_size = load_ramdisk(info->initrd_filename,
>>                                         info->initrd_start,
>> -                                       info->ram_size -
>> +                                       reg->base + reg->size -
>>                                         info->initrd_start);
>>              if (initrd_size < 0) {
>>                  initrd_size = load_image_targphys(info->initrd_filename,
>>                                                    info->initrd_start,
>> -                                                  info->ram_size -
>> +                                                  reg->base + reg->size -
>>                                                    info->initrd_start);
>>              }
>>              if (initrd_size < 0) {
>> @@ -1027,7 +1093,8 @@ static void arm_load_kernel_notify(Notifier *notifier, 
>> void *data)
>>              }
>>              fixupcontext[FIXUP_ARGPTR] = dtb_start;
>>          } else {
>> -            fixupcontext[FIXUP_ARGPTR] = info->loader_start + 
>> KERNEL_ARGS_ADDR;
>> +            hwaddr kernel_args_addr = info->loader_start + KERNEL_ARGS_ADDR;
>> +            fixupcontext[FIXUP_ARGPTR] = kernel_args_addr;
> What's this unrelated change for?

Sorry, it makes no sense.
>>              if (info->ram_size >= (1ULL << 32)) {
>>                  fprintf(stderr, "qemu: RAM size must be less than 4GB to 
>> boot"
>>                          " Linux kernel using ATAGS (try passing a device 
>> tree"
>> -- 
>> 1.8.3.1
>>
>>
>>
> Thanks,
> drew 
>
> .
>