| Message ID | 20180516152026.2920-5-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shameer, On 05/16/2018 05:20 PM, Shameer Kolothum wrote: > This makes changes to the DT mem node creation such that its easier > to add non-contiguous mem modeled as non-pluggable and a pc-dimm > mem later. See comments below. I think you should augment the description here with what the patch exactly adds: - a new helper function - a new dimm node? and if possible split functional changes into separate patches? > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > hw/arm/boot.c | 91 ++++++++++++++++++++++++++++++++++++---------------- > include/hw/arm/arm.h | 12 +++++++ > 2 files changed, 75 insertions(+), 28 deletions(-) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 26184bc..73db0aa 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -486,6 +486,27 @@ static void fdt_add_psci_node(void *fdt) > qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn); > } > > +static char *create_memory_fdt(void *fdt, uint32_t acells, hwaddr mem_base, > + uint32_t scells, hwaddr mem_len) Other dt node creation functions are named fdt_add_*_node > +{ > + char *nodename = NULL; > + int rc; > + > + 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); > + if (rc < 0) { > + fprintf(stderr, "couldn't set %s/reg\n", nodename); > + g_free(nodename); > + return NULL; > + } > + > + return nodename; > +} > + > + > /** > * load_dtb() - load a device tree binary image into memory > * @addr: the address to load the image at > @@ -567,50 +588,64 @@ 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 > + * /memory@addr nodes for all numa nodes respectively. > + */ > + qemu_fdt_nop_node(fdt, "/memory"); I don't really understand why this gets moved outside of the if (nb_numa_nodes > 0) { check. Also the comment above mention numa nodes whereas it are not necessarily in the numa case anymore. > + > if (nb_numa_nodes > 0) { > - /* > - * Turn the /memory node created before into a NOP node, then create > - * /memory@addr nodes for all numa nodes respectively. > - */ > - qemu_fdt_nop_node(fdt, "/memory"); > + hwaddr mem_sz; > + > mem_base = binfo->loader_start; > + mem_sz = binfo->ram_size; > 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, > + mem_len = MIN(numa_info[i].node_mem, mem_sz); I fail to understand how this change relates to the topic of this patch. If this adds a consistency check, this may be put in another patch? > + > + nodename = create_memory_fdt(fdt, acells, mem_base, > scells, mem_len); You could simplify the review by just introducing the new dt node creation function in a 1st patch and then introduce the changes to support non contiguous DT mem nodes. > - if (rc < 0) { > - fprintf(stderr, "couldn't set %s/reg for node %d\n", nodename, > - i); > + if (!nodename) { > goto fail; > } > > qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i); > - mem_base += mem_len; > g_free(nodename); > + mem_base += mem_len; > + mem_sz -= mem_len; > + if (!mem_sz) { > + break; So what does it mean practically if we break here. Not all the num nodes will function? Outputting a mesg for the end user may be useful. > + } > } > - } else { > - Error *err = NULL; > > - rc = fdt_path_offset(fdt, "/memory"); > - if (rc < 0) { > - qemu_fdt_add_subnode(fdt, "/memory"); > - } > + /* Create the node for initial pc-dimm ram, if any */ > + if (binfo->dimm_mem) { > > - if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) { > - qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory"); > + nodename = create_memory_fdt(fdt, acells, binfo->dimm_mem->base, > + scells, binfo->dimm_mem->size); > + if (!nodename) { > + goto fail; > + } > + qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", > + binfo->dimm_mem->node); > + g_free(nodename); > } > > - 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"); > + } else { > + > + nodename = create_memory_fdt(fdt, acells, binfo->loader_start, > + scells, binfo->ram_size); > + if (!nodename) { > goto fail; > } > + > + if (binfo->dimm_mem) { > + nodename = create_memory_fdt(fdt, acells, binfo->dimm_mem->base, > + scells, binfo->dimm_mem->size); > + if (!nodename) { > + goto fail; > + } > + g_free(nodename); > + } as this code gets duplicated, a helper function may be relevant? Thanks Eric > } > > rc = fdt_path_offset(fdt, "/chosen"); > diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h > index ce769bd..0ee3b4e 100644 > --- a/include/hw/arm/arm.h > +++ b/include/hw/arm/arm.h > @@ -48,6 +48,12 @@ typedef struct { > ARMCPU *cpu; /* handle to the first cpu object */ > } ArmLoadKernelNotifier; > > +struct dimm_mem_info { > + int node; > + hwaddr base; > + hwaddr size; > +}; > + > /* arm_boot.c */ > struct arm_boot_info { > uint64_t ram_size; > @@ -124,6 +130,12 @@ struct arm_boot_info { > bool secure_board_setup; > > arm_endianness endianness; > + > + /* This is used to model a pc-dimm based mem if the valid iova region > + * is non-contiguous. > + */ > + struct dimm_mem_info *dimm_mem; > + > }; > > /** >
Hi Eric, > -----Original Message----- > From: Auger Eric [mailto:eric.auger@redhat.com] > Sent: Monday, May 28, 2018 3:22 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > qemu-devel@nongnu.org; qemu-arm@nongnu.org > Cc: peter.maydell@linaro.org; drjones@redhat.com; Jonathan Cameron > <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>; > alex.williamson@redhat.com; Zhaoshenglong <zhaoshenglong@huawei.com>; > imammedo@redhat.com > Subject: Re: [Qemu-devel] [RFC v2 4/6] hw/arm: Changes required to > accommodate non-contiguous DT mem nodes > > Hi Shameer, > > On 05/16/2018 05:20 PM, Shameer Kolothum wrote: > > This makes changes to the DT mem node creation such that its easier > > to add non-contiguous mem modeled as non-pluggable and a pc-dimm > > mem later. > See comments below. I think you should augment the description here with > what the patch exactly adds: > - a new helper function > - a new dimm node? > > and if possible split functional changes into separate patches? Agree. It is better to split this. > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > hw/arm/boot.c | 91 ++++++++++++++++++++++++++++++++++++---------- > ------ > > include/hw/arm/arm.h | 12 +++++++ > > 2 files changed, 75 insertions(+), 28 deletions(-) > > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > > index 26184bc..73db0aa 100644 > > --- a/hw/arm/boot.c > > +++ b/hw/arm/boot.c > > @@ -486,6 +486,27 @@ static void fdt_add_psci_node(void *fdt) > > qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn); > > } > > > > +static char *create_memory_fdt(void *fdt, uint32_t acells, hwaddr > mem_base, > > + uint32_t scells, hwaddr mem_len) > Other dt node creation functions are named fdt_add_*_node Ok. > > +{ > > + char *nodename = NULL; > > + int rc; > > + > > + 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); > > + if (rc < 0) { > > + fprintf(stderr, "couldn't set %s/reg\n", nodename); > > + g_free(nodename); > > + return NULL; > > + } > > + > > + return nodename; > > +} > > + > > + > > /** > > * load_dtb() - load a device tree binary image into memory > > * @addr: the address to load the image at > > @@ -567,50 +588,64 @@ 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 > > + * /memory@addr nodes for all numa nodes respectively. > > + */ > > + qemu_fdt_nop_node(fdt, "/memory"); > I don't really understand why this gets moved outside of the if > (nb_numa_nodes > 0) { check. Also the comment above mention numa nodes > whereas it are not necessarily in the numa case anymore. Hmm..I think this is because virt.c has a create_fdt() where "/memory " subnode is created. May be I can keep that and update with non-plug mem and create a new "/memory @" for pc-dimm case. I will double check. > > + > > if (nb_numa_nodes > 0) { > > - /* > > - * Turn the /memory node created before into a NOP node, then create > > - * /memory@addr nodes for all numa nodes respectively. > > - */ > > - qemu_fdt_nop_node(fdt, "/memory"); > > + hwaddr mem_sz; > > + > > mem_base = binfo->loader_start; > > + mem_sz = binfo->ram_size; > > 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, > > + mem_len = MIN(numa_info[i].node_mem, mem_sz); > I fail to understand how this change relates to the topic of this patch. > If this adds a consistency check, this may be put in another patch? Yes. It is not related to this patch per se. But without this and some of the related mem_len/ mem_sz changes below it will end up creating more num of memory nodes as the numa mem info is populated much earlier than we modify the ram_size(non-plug) info. I can move this and your related comment for acpi patch (5/6) to a separate patch. > > + > > + nodename = create_memory_fdt(fdt, acells, mem_base, > > scells, mem_len); > You could simplify the review by just introducing the new dt node > creation function in a 1st patch and then introduce the changes to > support non contiguous DT mem nodes. Ok. I will split. > > - if (rc < 0) { > > - fprintf(stderr, "couldn't set %s/reg for node %d\n", nodename, > > - i); > > + if (!nodename) { > > goto fail; > > } > > > > qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i); > > - mem_base += mem_len; > > g_free(nodename); > > + mem_base += mem_len; > > + mem_sz -= mem_len; > > + if (!mem_sz) { > > + break; > So what does it mean practically if we break here. Not all the num nodes > will function? Outputting a mesg for the end user may be useful. The break here means we have populated mem nodes for non-plug ram case and the remaining mem(if any) will be based on the pc-dimm case. Yes, it is possible that Guest kernel will report memory-less numa nodes. I will add a msg here. > > + } > > } > > - } else { > > - Error *err = NULL; > > > > - rc = fdt_path_offset(fdt, "/memory"); > > - if (rc < 0) { > > - qemu_fdt_add_subnode(fdt, "/memory"); > > - } > > + /* Create the node for initial pc-dimm ram, if any */ > > + if (binfo->dimm_mem) { > > > > - if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) { > > - qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory"); > > + nodename = create_memory_fdt(fdt, acells, binfo->dimm_mem- > >base, > > + scells, binfo->dimm_mem->size); > > + if (!nodename) { > > + goto fail; > > + } > > + qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", > > + binfo->dimm_mem->node); > > + g_free(nodename); > > } > > > > - 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"); > > + } else { > > + > > + nodename = create_memory_fdt(fdt, acells, binfo->loader_start, > > + scells, binfo->ram_size); > > + if (!nodename) { > > goto fail; > > } > > + > > + if (binfo->dimm_mem) { > > + nodename = create_memory_fdt(fdt, acells, binfo->dimm_mem- > >base, > > + scells, binfo->dimm_mem->size); > > + if (!nodename) { > > + goto fail; > > + } > > + g_free(nodename); > > + } > as this code gets duplicated, a helper function may be relevant? Ok. Thanks, Shameer > Thanks > > Eric > > } > > > > rc = fdt_path_offset(fdt, "/chosen"); > > diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h > > index ce769bd..0ee3b4e 100644 > > --- a/include/hw/arm/arm.h > > +++ b/include/hw/arm/arm.h > > @@ -48,6 +48,12 @@ typedef struct { > > ARMCPU *cpu; /* handle to the first cpu object */ > > } ArmLoadKernelNotifier; > > > > +struct dimm_mem_info { > > + int node; > > + hwaddr base; > > + hwaddr size; > > +}; > > + > > /* arm_boot.c */ > > struct arm_boot_info { > > uint64_t ram_size; > > @@ -124,6 +130,12 @@ struct arm_boot_info { > > bool secure_board_setup; > > > > arm_endianness endianness; > > + > > + /* This is used to model a pc-dimm based mem if the valid iova region > > + * is non-contiguous. > > + */ > > + struct dimm_mem_info *dimm_mem; > > + > > }; > > > > /** > >
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 26184bc..73db0aa 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -486,6 +486,27 @@ static void fdt_add_psci_node(void *fdt) qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn); } +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("/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); + if (rc < 0) { + fprintf(stderr, "couldn't set %s/reg\n", nodename); + g_free(nodename); + return NULL; + } + + return nodename; +} + + /** * load_dtb() - load a device tree binary image into memory * @addr: the address to load the image at @@ -567,50 +588,64 @@ 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 + * /memory@addr 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 - * /memory@addr nodes for all numa nodes respectively. - */ - qemu_fdt_nop_node(fdt, "/memory"); + hwaddr mem_sz; + mem_base = binfo->loader_start; + mem_sz = binfo->ram_size; 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, + mem_len = MIN(numa_info[i].node_mem, mem_sz); + + nodename = create_memory_fdt(fdt, acells, mem_base, scells, mem_len); - if (rc < 0) { - fprintf(stderr, "couldn't set %s/reg for node %d\n", nodename, - i); + if (!nodename) { goto fail; } qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i); - mem_base += mem_len; g_free(nodename); + mem_base += mem_len; + mem_sz -= mem_len; + if (!mem_sz) { + break; + } } - } else { - Error *err = NULL; - rc = fdt_path_offset(fdt, "/memory"); - if (rc < 0) { - qemu_fdt_add_subnode(fdt, "/memory"); - } + /* Create the node for initial pc-dimm ram, if any */ + if (binfo->dimm_mem) { - if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) { - qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory"); + nodename = create_memory_fdt(fdt, acells, binfo->dimm_mem->base, + scells, binfo->dimm_mem->size); + if (!nodename) { + goto fail; + } + qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", + binfo->dimm_mem->node); + g_free(nodename); } - 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"); + } else { + + nodename = create_memory_fdt(fdt, acells, binfo->loader_start, + scells, binfo->ram_size); + if (!nodename) { goto fail; } + + if (binfo->dimm_mem) { + nodename = create_memory_fdt(fdt, acells, binfo->dimm_mem->base, + scells, binfo->dimm_mem->size); + if (!nodename) { + goto fail; + } + g_free(nodename); + } } rc = fdt_path_offset(fdt, "/chosen"); diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h index ce769bd..0ee3b4e 100644 --- a/include/hw/arm/arm.h +++ b/include/hw/arm/arm.h @@ -48,6 +48,12 @@ typedef struct { ARMCPU *cpu; /* handle to the first cpu object */ } ArmLoadKernelNotifier; +struct dimm_mem_info { + int node; + hwaddr base; + hwaddr size; +}; + /* arm_boot.c */ struct arm_boot_info { uint64_t ram_size; @@ -124,6 +130,12 @@ struct arm_boot_info { bool secure_board_setup; arm_endianness endianness; + + /* This is used to model a pc-dimm based mem if the valid iova region + * is non-contiguous. + */ + struct dimm_mem_info *dimm_mem; + }; /**
This makes changes to the DT mem node creation such that its easier to add non-contiguous mem modeled as non-pluggable and a pc-dimm mem later. Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- hw/arm/boot.c | 91 ++++++++++++++++++++++++++++++++++++---------------- include/hw/arm/arm.h | 12 +++++++ 2 files changed, 75 insertions(+), 28 deletions(-)