Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 18 October 2019 17:40
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> Hi Shameer,
> 
> On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> > This series adds NVDIMM support to arm/virt platform.
> > This has a dependency on [0] and make use of the GED
> > device for NVDIMM hotplug events. The series reuses
> > some of the patches posted by Eric in his earlier
> > attempt here[1].
> >
> > Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> > hot add case described here[2].
> >
> > I have done basic sanity testing of NVDIMM deviecs with
> devcies
> > both ACPI and DT Guest boot. Further testing is always
> > welcome.
> >
> > Please let me know your feedback.
> 
> I tested it on my side. Looks to work pretty well.

Thanks for giving this a spin.
 
> one question: I noticed that when a NVDIMM slot is hotplugged one get
> the following trace on guest:
> 
> nfit ACPI0012:00: found a zero length table '0' parsing nfit
> pmem0: detected capacity change from 0 to 1073741824
> 
> Have you experienced the 0 length trace?

I double checked and yes that trace is there. And I did a quick check with
x86 and it is not there. 

The reason looks like, ARM64 kernel receives an additional 8 bytes size when
the kernel evaluates the "_FIT" object. 

For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and 

X86 Guest kernel,
[    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8

ARM64 Guest,
[    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0

I am not sure how that size gets changed for ARM which results in
the above mentioned 0 length trace. I need to debug this further.

Please let me know if you have any pointers...
 
> Besides when we reset the system we find the namespaces again using
> "ndctl list -u" so the original bug seems to be fixed.
> 
> Did you try to mount a DAX FS. I can mount but with DAX forced off.
> sudo mkdir /mnt/mem0

Yes. I did try with DAX FS. But do we need to change the namespace mode to 
file system DAX mode?

I used the below command before attempting to mount with -o dax,

ndctl create-namespace -f -e namespace0.0 --mode=fsdax

And in order to do the above you might need the ZONE_DEVICE support
in the Kernel which in turn has dependency on hot remove. Hence I tried with
"arm64/mm: Enable memory hot remove" patches,

https://patchwork.kernel.org/cover/11185169/

> mkfs.xfs -f -m reflink=0 /dev/pmem0
> sudo mount -o dax /dev/pmem0 /mnt/mem0
> [ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
> your own risk
> [ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
> DAX.
> [ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
> [ 2610.189797] XFS (pmem0): Ending clean mount
> 
> I fail to remember if it was the case months ago. I am not sure if it is
> an issue in my guest .config or if there is something not yet supported
> on aarch64? Did you try on your side?
> 
> Also if you forget to put the ",nvdimm" to the machvirt options you get,
> on hotplug:
> {"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
> which is not correct anymore ;-)

Ok. I will check this.

Thanks,
Shameer
 
> Thanks
> 
> Eric
> 
> 
> >
> > Thanks,
> > Shameer
> >
> > [0] https://patchwork.kernel.org/cover/11150345/
> > [1] https://patchwork.kernel.org/cover/10830777/
> > [2] https://patchwork.kernel.org/patch/11154757/
> >
> > Eric Auger (1):
> >   hw/arm/boot: Expose the pmem nodes in the DT
> >
> > Kwangwoo Lee (2):
> >   nvdimm: Use configurable ACPI IO base and size
> >   hw/arm/virt: Add nvdimm hot-plug infrastructure
> >
> > Shameer Kolothum (2):
> >   hw/arm: Align ACPI blob len to PAGE size
> >   hw/arm/virt: Add nvdimm hotplug support
> >
> >  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
> >  hw/acpi/generic_event_device.c         | 13 ++++++++
> >  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
> >  hw/arm/Kconfig                         |  1 +
> >  hw/arm/boot.c                          | 45
> ++++++++++++++++++++++++++
> >  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
> >  hw/arm/virt.c                          | 42
> ++++++++++++++++++++----
> >  hw/i386/acpi-build.c                   |  6 ++++
> >  hw/i386/acpi-build.h                   |  3 ++
> >  hw/i386/pc_piix.c                      |  2 ++
> >  hw/i386/pc_q35.c                       |  2 ++
> >  hw/mem/Kconfig                         |  2 +-
> >  include/hw/acpi/generic_event_device.h |  1 +
> >  include/hw/arm/virt.h                  |  1 +
> >  include/hw/mem/nvdimm.h                |  3 ++
> >  15 files changed, 157 insertions(+), 17 deletions(-)
> >