[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v6 03/18] hw/arm/boot: introduce fdt_add_memory_no
From: |
Peter Maydell |
Subject: |
Re: [Qemu-arm] [PATCH v6 03/18] hw/arm/boot: introduce fdt_add_memory_node helper |
Date: |
Thu, 14 Feb 2019 16:49:11 +0000 |
On Tue, 5 Feb 2019 at 17:33, Eric Auger <address@hidden> wrote:
>
> From: Shameer Kolothum <address@hidden>
>
> We introduce an helper to create a memory node.
>
> Signed-off-by: Eric Auger <address@hidden>
> Signed-off-by: Shameer Kolothum <address@hidden>
> ---
> hw/arm/boot.c | 54 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 05762d0fc1..2ef367e15b 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -423,6 +423,36 @@ static void set_kernel_args_old(const struct
> arm_boot_info *info,
> }
> }
>
> +static int fdt_add_memory_node(void *fdt, uint32_t acells, hwaddr mem_base,
> + uint32_t scells, hwaddr mem_len,
> + int numa_node_id)
> +{
> + char *nodename = NULL;
You set nodename immediately below, so no need for the NULL initialization here.
> + int ret;
> +
> + nodename = g_strdup_printf("/address@hidden" PRIx64, mem_base);
> + qemu_fdt_add_subnode(fdt, nodename);
> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
> + ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells,
> mem_base,
> + scells, mem_len);
> + if (ret < 0) {
> + fprintf(stderr, "couldn't set %s/reg\n", nodename);
> + goto out;
> + }
I think error handling (ie whether we print messages or not) ought to be
done by the calling function, rather than here.
> + if (numa_node_id < 0) {
What is this for? My original theory was that this was an error
case that should probably be an assert(), but we seem to use it
in one of the callers below. A brief comment at the top of the
function documenting its API would assist here. If this is
"only set the NUMA ID if it is specified" then I think writing it as
if (numa_node_id >= 0) {
set the id;
}
is clearer than making it look like an error-exit check.
> + goto out;
> + }
> +
> + ret = qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", numa_node_id);
> + if (ret < 0) {
> + fprintf(stderr, "couldn't set %s/numa-node-id\n", nodename);
> + }
> +
> +out:
> + g_free(nodename);
> + return ret;
> +}
> +
> static void fdt_add_psci_node(void *fdt)
> {
> uint32_t cpu_suspend_fn;
> @@ -502,7 +532,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
> *binfo,
> void *fdt = NULL;
> int size, rc, n = 0;
> uint32_t acells, scells;
> - char *nodename;
> unsigned int i;
> hwaddr mem_base, mem_len;
> char **node_path;
> @@ -576,35 +605,20 @@ int arm_load_dtb(hwaddr addr, const struct
> arm_boot_info *binfo,
> mem_base = binfo->loader_start;
> 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);
> + rc = fdt_add_memory_node(fdt, acells, mem_base,
> + scells, mem_len, i);
> if (rc < 0) {
> - fprintf(stderr, "couldn't set %s/reg for node %d\n",
> nodename,
> - i);
> goto fail;
> }
>
> - qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
> mem_base += mem_len;
> - g_free(nodename);
> }
> } else {
> - nodename = g_strdup_printf("/address@hidden" PRIx64,
> binfo->loader_start);
> - 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, binfo->loader_start,
> - scells, binfo->ram_size);
> + rc = fdt_add_memory_node(fdt, acells, binfo->loader_start,
> + scells, binfo->ram_size, -1);
> if (rc < 0) {
> - fprintf(stderr, "couldn't set %s reg\n", nodename);
> goto fail;
> }
> - g_free(nodename);
> }
>
> rc = fdt_path_offset(fdt, "/chosen");
> --
> 2.20.1
>
thanks
-- PMM
- [Qemu-arm] [PATCH v6 00/18] ARM virt: Initial RAM expansion and PCDIMM/NVDIMM support, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 01/18] update-linux-headers.sh: Copy new headers, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 04/18] hw/arm/virt: Rename highmem IO regions, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 06/18] hw/boards: Add a MachineState parameter to kvm_type callback, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 03/18] hw/arm/boot: introduce fdt_add_memory_node helper, Eric Auger, 2019/02/05
- Re: [Qemu-arm] [PATCH v6 03/18] hw/arm/boot: introduce fdt_add_memory_node helper,
Peter Maydell <=
- [Qemu-arm] [PATCH v6 05/18] hw/arm/virt: Split the memory map description, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 07/18] kvm: add kvm_arm_get_max_vm_phys_shift, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 08/18] vl: Set machine ram_size, maxram_size and ram_slots earlier, Eric Auger, 2019/02/05
- [Qemu-arm] [PATCH v6 09/18] hw/arm/virt: Implement kvm_type function for 4.0 machine, Eric Auger, 2019/02/05