| Message ID | 20190308114218.26692-9-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM virt: ACPI memory hotplug support | expand |
On Fri, 8 Mar 2019 11:42:15 +0000 Shameer Kolothum <shameerali.kolothum.thodi@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.thodi@huawei.com> > Signed-off-by: Eric Auger <eric.auger@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. > + 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");
> -----Original Message----- > From: Igor Mammedov [mailto:imammedo@redhat.com] > Sent: 11 March 2019 14:59 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > eric.auger@redhat.com; peter.maydell@linaro.org; > shannon.zhaosl@gmail.com; sameo@linux.intel.com; > sebastien.boeuf@intel.com; Linuxarm <linuxarm@huawei.com>; xuwei (O) > <xuwei5@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.thodi@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.thodi@huawei.com> > > Signed-off-by: Eric Auger <eric.auger@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. 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");
Hi Shameer, On 3/12/19 4:13 PM, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: Igor Mammedov [mailto:imammedo@redhat.com] >> Sent: 11 March 2019 14:59 >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> >> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; >> eric.auger@redhat.com; peter.maydell@linaro.org; >> shannon.zhaosl@gmail.com; sameo@linux.intel.com; >> sebastien.boeuf@intel.com; Linuxarm <linuxarm@huawei.com>; xuwei (O) >> <xuwei5@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.thodi@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.thodi@huawei.com> >>> Signed-off-by: Eric Auger <eric.auger@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. 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"); > >
> -----Original Message----- > From: Auger Eric [mailto:eric.auger@redhat.com] > Sent: 12 March 2019 15:38 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > Igor Mammedov <imammedo@redhat.com> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com; > shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm > <linuxarm@huawei.com>; qemu-arm@nongnu.org; xuwei (O) > <xuwei5@huawei.com>; sebastien.boeuf@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:imammedo@redhat.com] > >> Sent: 11 March 2019 14:59 > >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > >> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > >> eric.auger@redhat.com; peter.maydell@linaro.org; > >> shannon.zhaosl@gmail.com; sameo@linux.intel.com; > >> sebastien.boeuf@intel.com; Linuxarm <linuxarm@huawei.com>; xuwei (O) > >> <xuwei5@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.thodi@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.thodi@huawei.com> > >>> Signed-off-by: Eric Auger <eric.auger@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(). 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"); > > > >
Hi Shameer, On 3/12/19 5:39 PM, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: Auger Eric [mailto:eric.auger@redhat.com] >> Sent: 12 March 2019 15:38 >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; >> Igor Mammedov <imammedo@redhat.com> >> Cc: peter.maydell@linaro.org; sameo@linux.intel.com; >> shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm >> <linuxarm@huawei.com>; qemu-arm@nongnu.org; xuwei (O) >> <xuwei5@huawei.com>; sebastien.boeuf@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:imammedo@redhat.com] >>>> Sent: 11 March 2019 14:59 >>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> >>>> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; >>>> eric.auger@redhat.com; peter.maydell@linaro.org; >>>> shannon.zhaosl@gmail.com; sameo@linux.intel.com; >>>> sebastien.boeuf@intel.com; Linuxarm <linuxarm@huawei.com>; xuwei (O) >>>> <xuwei5@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.thodi@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.thodi@huawei.com> >>>>> Signed-off-by: Eric Auger <eric.auger@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? 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"); >>> >>>
Hi Eric, > -----Original Message----- > From: Auger Eric [mailto:eric.auger@redhat.com] > Sent: 12 March 2019 16:48 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > Igor Mammedov <imammedo@redhat.com> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com; > shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm > <linuxarm@huawei.com>; qemu-arm@nongnu.org; xuwei (O) > <xuwei5@huawei.com>; sebastien.boeuf@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.auger@redhat.com] > >> Sent: 12 March 2019 15:38 > >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > >> Igor Mammedov <imammedo@redhat.com> > >> Cc: peter.maydell@linaro.org; sameo@linux.intel.com; > >> shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm > >> <linuxarm@huawei.com>; qemu-arm@nongnu.org; xuwei (O) > >> <xuwei5@huawei.com>; sebastien.boeuf@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:imammedo@redhat.com] > >>>> Sent: 11 March 2019 14:59 > >>>> To: Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com> > >>>> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > >>>> eric.auger@redhat.com; peter.maydell@linaro.org; > >>>> shannon.zhaosl@gmail.com; sameo@linux.intel.com; > >>>> sebastien.boeuf@intel.com; Linuxarm <linuxarm@huawei.com>; xuwei > (O) > >>>> <xuwei5@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.thodi@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.thodi@huawei.com> > >>>>> Signed-off-by: Eric Auger <eric.auger@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"); > >>> > >>>
Hi Shameer, On 3/12/19 5:56 PM, Shameerali Kolothum Thodi wrote: > Hi Eric, > >> -----Original Message----- >> From: Auger Eric [mailto:eric.auger@redhat.com] >> Sent: 12 March 2019 16:48 >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; >> Igor Mammedov <imammedo@redhat.com> >> Cc: peter.maydell@linaro.org; sameo@linux.intel.com; >> shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm >> <linuxarm@huawei.com>; qemu-arm@nongnu.org; xuwei (O) >> <xuwei5@huawei.com>; sebastien.boeuf@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.auger@redhat.com] >>>> Sent: 12 March 2019 15:38 >>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; >>>> Igor Mammedov <imammedo@redhat.com> >>>> Cc: peter.maydell@linaro.org; sameo@linux.intel.com; >>>> shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm >>>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; xuwei (O) >>>> <xuwei5@huawei.com>; sebastien.boeuf@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:imammedo@redhat.com] >>>>>> Sent: 11 March 2019 14:59 >>>>>> To: Shameerali Kolothum Thodi >> <shameerali.kolothum.thodi@huawei.com> >>>>>> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; >>>>>> eric.auger@redhat.com; peter.maydell@linaro.org; >>>>>> shannon.zhaosl@gmail.com; sameo@linux.intel.com; >>>>>> sebastien.boeuf@intel.com; Linuxarm <linuxarm@huawei.com>; xuwei >> (O) >>>>>> <xuwei5@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.thodi@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.thodi@huawei.com> >>>>>>> Signed-off-by: Eric Auger <eric.auger@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"); >>>>> >>>>>
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); + 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");