Re: [Qemu-arm] [PATCH v2 08/11] hw/arm/boot: Expose the PC-DIMM nodes in the DT

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: 11 March 2019 14:59
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; Linuxarm <address@hidden>; xuwei (O)
> <address@hidden>
> Subject: Re: [PATCH v2 08/11] hw/arm/boot: Expose the PC-DIMM nodes in the
> DT
> 
> On Fri, 8 Mar 2019 11:42:15 +0000
> Shameer Kolothum <address@hidden> wrote:
> 
> > This patch adds memory nodes corresponding to PC-DIMM regions.
> >
> > NVDIMM and ACPI_NVDIMM configs are not yet set for ARM so we
> > don't need to care about NVDIMM at this stage.
> >
> > Signed-off-by: Shameer Kolothum <address@hidden>
> > Signed-off-by: Eric Auger <address@hidden>
> > ---
> >  hw/arm/boot.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index a830655..4caaf91 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -19,6 +19,7 @@
> >  #include "sysemu/numa.h"
> >  #include "hw/boards.h"
> >  #include "hw/loader.h"
> > +#include "hw/mem/memory-device.h"
> >  #include "elf.h"
> >  #include "sysemu/device_tree.h"
> >  #include "qemu/config-file.h"
> > @@ -522,6 +523,41 @@ static void fdt_add_psci_node(void *fdt)
> >      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
> >  }
> >
> > +static int fdt_add_hotpluggable_memory_nodes(void *fdt,
> > +                                             uint32_t acells,
> uint32_t scells) {
> > +    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
> > +    MemoryDeviceInfo *mi;
> > +    int ret = 0;
> > +
> > +    for (info = info_list; info != NULL; info = info->next) {
> > +        mi = info->value;
> > +        switch (mi->type) {
> > +        case MEMORY_DEVICE_INFO_KIND_DIMM:
> > +        {
> > +            PCDIMMDeviceInfo *di = mi->u.dimm.data;
> > +
> > +            ret = fdt_add_memory_node(fdt, acells, di->addr,
> > +                                      scells, di->size, di->node);
> > +            if (ret) {
> > +                fprintf(stderr,
> > +                        "couldn't add PCDIMM /address@hidden"PRIx64"
> node\n",
> > +                        di->addr);
> > +                goto out;
> > +            }
> > +            break;
> > +        }
> > +        default:
> > +            fprintf(stderr, "%s memory nodes are not yet supported\n",
> > +                    MemoryDeviceInfoKind_str(mi->type));
> > +            ret = -ENOENT;
> > +            goto out;
> > +        }
> > +    }
> > +out:
> > +    qapi_free_MemoryDeviceInfoList(info_list);
> > +    return ret;
> > +}
> > +
> >  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >                   hwaddr addr_limit, AddressSpace *as)
> >  {
> > @@ -621,6 +657,12 @@ int arm_load_dtb(hwaddr addr, const struct
> arm_boot_info *binfo,
> >          }
> >      }
> >
> > +    rc = fdt_add_hotpluggable_memory_nodes(fdt, acells, scells);
> According to Eric's test, guest kernel picks this up. So it probably
> should be an opt-in feature to avoid conflict with ACPI definition in DSDT.

Ok. My take away from that discussion was that if we properly describe the
PNP0C80 (which is what #06 of this series does) then we don't have to worry
about DT entries as ACPI will "override" it.  But it looks like that is not the 
case
and describing DT will anyway cause the memory being end up in UEFI
GeTMemoryMap() and results in being used as normal memory early at boot.

Hi Eric,

How do we verify that this is actually happening? I tried running the 
/proc/meminfo
on a Qemu build upto #06 of this series, and it reported memory including the
cold plugged dimm memory.

But not sure how to verify that this dimm mem is being treated as normal mem on
boot time or not? Please let me know.

Thanks,
Shameer


> > +    if (rc < 0) {
> > +        fprintf(stderr, "couldn't add hotpluggable memory nodes\n");
> > +        goto fail;
> > +    }
> > +
> >      rc = fdt_path_offset(fdt, "/chosen");
> >      if (rc < 0) {
> >          qemu_fdt_add_subnode(fdt, "/chosen");