Hi Eric,
> -----Original Message-----
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: Monday, May 28, 2018 3:22 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>;
> qemu-devel@nongnu.org; qemu-...@nongnu.org
> Cc: peter.mayd...@linaro.org; drjo...@redhat.com; Jonathan Cameron
> <jonathan.came...@huawei.com>; Linuxarm <linux...@huawei.com>;
> alex.william...@redhat.com; Zhaoshenglong <zhaoshengl...@huawei.com>;
> imamm...@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.th...@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;
> > +
> > };
> >
> > /**
> >
