Hi Shameer,
On 3/12/19 5:56 PM, Shameerali Kolothum Thodi wrote:
> 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.
OK I get it. Thank you for your patience! Maybe actual status can be
checked in /sys/devices/system/memory/memoryX/*?
Thanks
Eric
>>>
>>> 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");
>>>>>
>>>>>
