Re: [Qemu-devel] [RFC v3 08/15] hw/arm/boot: introduce fdt_add_memory_node helper
On Wed, 8 Aug 2018 11:44:14 +0200
Auger Eric wrote:
> Hi Igor,
>
> On 07/18/2018 04:04 PM, Igor Mammedov wrote:
> > On Tue, 3 Jul 2018 09:19:51 +0200
> > Eric Auger wrote:
> >
> >> From: Shameer Kolothum
> >>
> >> We introduce an helper to create a memory node.
> >>
> >> Signed-off-by: Eric Auger
> >> Signed-off-by: Shameer Kolothum
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - nop of existing /memory nodes was already handled
> >> ---
> >> 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 e09201c..5243a25 100644
> >> --- a/hw/arm/boot.c
> >> +++ b/hw/arm/boot.c
> >> @@ -413,6 +413,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;
> >> +int ret;
> >> +
> >> +nodename = g_strdup_printf("/memory@%" 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;
> >> +}
> >> +if (numa_node_id < 0) {
> >> +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;
> >> +}
> >> +
> >
> > not related question from hotplug POV,
> > is entry size fixed?
> Sorry I don't get what entry you are referring to?
> > can we estimate exact size for #slots number of dimms and reserve it in
> > advance
> > in FDT 'rom'?
> Not sure I get your drift either.
>
> patch "[RFC v3 09/15] hw/arm/boot: Expose the PC-DIMM nodes in the DT"
> builds the DT nodes for each node, by enumerating the MemoryDeviceInfoList.
In case of hotplug we don not care about adding DTB node at runtime
(guest won't see it anyways). However if we reboot machine it's reasonable
to regenerate DTB on reboot so guest would see previously hotplugged DIMMs.
Problem is that DTB is stored in fixed size 'rom' that's copied into guest's
RAM, so we should reserve a space for possible slots in advance or switch
to another mechanism to provide DTB to guest. (it could be a memory region
mapped outside of RAM)
[...]
Re: [Qemu-devel] [RFC v3 08/15] hw/arm/boot: introduce fdt_add_memory_node helper
Hi Igor,
On 07/18/2018 04:04 PM, Igor Mammedov wrote:
> On Tue, 3 Jul 2018 09:19:51 +0200
> Eric Auger wrote:
>
>> From: Shameer Kolothum
>>
>> We introduce an helper to create a memory node.
>>
>> Signed-off-by: Eric Auger
>> Signed-off-by: Shameer Kolothum
>>
>> ---
>>
>> v1 -> v2:
>> - nop of existing /memory nodes was already handled
>> ---
>> 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 e09201c..5243a25 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -413,6 +413,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;
>> +int ret;
>> +
>> +nodename = g_strdup_printf("/memory@%" 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;
>> +}
>> +if (numa_node_id < 0) {
>> +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;
>> +}
>> +
>
> not related question from hotplug POV,
> is entry size fixed?
Sorry I don't get what entry you are referring to?
> can we estimate exact size for #slots number of dimms and reserve it in
> advance
> in FDT 'rom'?
Not sure I get your drift either.
patch "[RFC v3 09/15] hw/arm/boot: Expose the PC-DIMM nodes in the DT"
builds the DT nodes for each node, by enumerating the MemoryDeviceInfoList.
>
>> static void fdt_add_psci_node(void *fdt)
>> {
>> uint32_t cpu_suspend_fn;
>> @@ -492,7 +522,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;
>> @@ -566,35 +595,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("/memory@%" 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("/memory@%" 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");
> nice cleanup, but I won't stop here just yet if hotplug to be considered.
>
> I see arm_load_dtb() as a hack called from every board
> where we dump everything that might be related to DTB regardless
> if it's generic for every board or only a board specific stuff.
>
> Could we split it in several logical parts that do a single thing
> and preferably user only when they are actually need?
> Something along following lines:
> (cleanups/refactoring should be a separate from pcdimm series as
Re: [Qemu-devel] [RFC v3 08/15] hw/arm/boot: introduce fdt_add_memory_node helper
On Tue, 3 Jul 2018 09:19:51 +0200
Eric Auger wrote:
> From: Shameer Kolothum
>
> We introduce an helper to create a memory node.
>
> Signed-off-by: Eric Auger
> Signed-off-by: Shameer Kolothum
>
> ---
>
> v1 -> v2:
> - nop of existing /memory nodes was already handled
> ---
> 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 e09201c..5243a25 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -413,6 +413,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;
> +int ret;
> +
> +nodename = g_strdup_printf("/memory@%" 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;
> +}
> +if (numa_node_id < 0) {
> +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;
> +}
> +
not related question from hotplug POV,
is entry size fixed?
can we estimate exact size for #slots number of dimms and reserve it in advance
in FDT 'rom'?
> static void fdt_add_psci_node(void *fdt)
> {
> uint32_t cpu_suspend_fn;
> @@ -492,7 +522,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;
> @@ -566,35 +595,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("/memory@%" 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("/memory@%" 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");
nice cleanup, but I won't stop here just yet if hotplug to be considered.
I see arm_load_dtb() as a hack called from every board
where we dump everything that might be related to DTB regardless
if it's generic for every board or only a board specific stuff.
Could we split it in several logical parts that do a single thing
and preferably user only when they are actually need?
Something along following lines:
(cleanups/refactoring should be a separate from pcdimm series as it's self
sufficient
and it would be easier to review/merge and could simplify following up pcdimm
series):
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e09201c..9c41efd 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@ -486,9 +486,6 @@ static void fdt_add_psci_node(void *fdt)
qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
}
-int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
-
[Qemu-devel] [RFC v3 08/15] hw/arm/boot: introduce fdt_add_memory_node helper
From: Shameer Kolothum
We introduce an helper to create a memory node.
Signed-off-by: Eric Auger
Signed-off-by: Shameer Kolothum
---
v1 -> v2:
- nop of existing /memory nodes was already handled
---
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 e09201c..5243a25 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -413,6 +413,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;
+int ret;
+
+nodename = g_strdup_printf("/memory@%" 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;
+}
+if (numa_node_id < 0) {
+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;
@@ -492,7 +522,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;
@@ -566,35 +595,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("/memory@%" 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("/memory@%" 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.5.5
