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

[Auger Eric]

Hi Shameer,

On 3/12/19 4:13 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----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.

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.

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");
> 
>