Hi Eric,
> -----Original Message-----
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: 12 March 2019 16:48
> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>;
> Igor Mammedov <imamm...@redhat.com>
> Cc: peter.mayd...@linaro.org; sa...@linux.intel.com;
> shannon.zha...@gmail.com; qemu-devel@nongnu.org; Linuxarm
> <linux...@huawei.com>; qemu-...@nongnu.org; xuwei (O)
> <xuw...@huawei.com>; sebastien.bo...@intel.com
> Subject: Re: [Qemu-devel] [PATCH v2 08/11] hw/arm/boot: Expose the
> PC-DIMM nodes in the DT
>
> Hi Shameer,
>
> On 3/12/19 5:39 PM, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Auger Eric [mailto:eric.au...@redhat.com]
> >> Sent: 12 March 2019 15:38
> >> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>;
> >> Igor Mammedov <imamm...@redhat.com>
> >> Cc: peter.mayd...@linaro.org; sa...@linux.intel.com;
> >> shannon.zha...@gmail.com; qemu-devel@nongnu.org; Linuxarm
> >> <linux...@huawei.com>; qemu-...@nongnu.org; xuwei (O)
> >> <xuw...@huawei.com>; sebastien.bo...@intel.com
> >> Subject: Re: [Qemu-devel] [PATCH v2 08/11] hw/arm/boot: Expose the
> >> PC-DIMM nodes in the DT
> >>
> >> Hi Shameer,
> >>
> >> On 3/12/19 4:13 PM, Shameerali Kolothum Thodi wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Igor Mammedov [mailto:imamm...@redhat.com]
> >>>> Sent: 11 March 2019 14:59
> >>>> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>
> >>>> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> >>>> eric.au...@redhat.com; peter.mayd...@linaro.org;
> >>>> shannon.zha...@gmail.com; sa...@linux.intel.com;
> >>>> sebastien.bo...@intel.com; Linuxarm <linux...@huawei.com>; xuwei
> (O)
> >>>> <xuw...@huawei.com>
> >>>> 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 <shameerali.kolothum.th...@huawei.com> 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
> >> <shameerali.kolothum.th...@huawei.com>
> >>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
> >>>>> ---
> >>>>> 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
> >> /memory@%"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.
> >>
> >> At the time I tested I did not have PNP0C80 build. I confirm that if you
> >> remove the dt description in ACPI mode, /proc/meminfo does not report
> >> the DIMMs. Now that you have #06, if you remove the DT hotpluggables
> >> memory mode generation in ACPI mode, you should see them and if I
> >> understand correctly this is what you confirm. I have not tested
> >> anything else at the moment.
> >
> > Yes, that is what I confirmed. But the concern now is having DT memory node
> > generation will result in the problem described above - UEFI
> GetMemoryMap().
> Sorry I don't understand the stated problem. If you remove the call to
> fdt_add_hotpluggable_memory_nodes when running in ACPI mode, does the
> above GetMemoryMap see the DIMMs?
That is what I am also trying to verify :). How do we verify the GetMemoryMap()
sees
the DIMMs or not? As I said, I have tested it with patch upto #06(means no
fdt_add_hotpluggable_memory_nodes) and checked /proc/meminfo which has
the dimm memory included in the total memory it reports.
But not sure how you will verify whether GetMemoryMap() see the dimm or not.
Thanks,
Shameer
> Thanks
>
> Eric
> >
> > I was looking for a way to check that is a valid issue or not. I will try
> > to look for
> > a way to verify that. Please update if you come across anything.
> >
> > Thanks,
> > Shameer
> >
> >> Thanks
> >>
> >> Eric
> >>>
> >>> 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");
> >>>
> >>>
