[0/5] ARM virt: Add NVDIMM support

Message

Shameerali Kolothum Thodi Oct. 4, 2019, 3:52 p.m. UTC

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
both ACPI and DT Guest boot. Further testing is always
welcome.

Please let me know your feedback.

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(-)

Comments

Eric Auger Oct. 18, 2019, 4:39 p.m. UTC | #1

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.

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?

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

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(-)
>

Shameerali Kolothum Thodi Oct. 22, 2019, 2:05 p.m. UTC | #2

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:[e-mail]]
> Sent: 18 October 2019 17:40
> To: Shameerali Kolothum Thodi <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; xuwei (O)
> <[e-mail]>; [e-mail]; Linuxarm
> <[e-mail]>
> 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(-)
> >

Igor Mammedov Nov. 12, 2019, 2:39 p.m. UTC | #3

On Fri, 4 Oct 2019 16:52:57 +0100
Shameer Kolothum <[e-mail]> 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
> both ACPI and DT Guest boot. Further testing is always
> welcome.
> 
> Please let me know your feedback.
one more thing,

It's possible to run bios-tables-test for virt/arm now,
pls add corresponding test case
it might be easier to just extend test_acpi_virt_tcg_memhp() with nvdimms

> 
> 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(-)
>

Shameerali Kolothum Thodi Nov. 25, 2019, 1:20 p.m. UTC | #4

Hi Eric/Igor,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 22 October 2019 15:05
> To: 'Auger Eric' <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; xuwei (O)
> <[e-mail]>; [e-mail]; Linuxarm
> <[e-mail]>
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 

[..]

> > 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...

I spend some time debugging this further and it looks like the AML code
behaves differently on x86 and ARM64.

Booted guest with nvdimm mem, and used SSDT override with dbg prints
added,

-object memory-backend-ram,id=mem1,size=1G \
-device nvdimm,id=dimm1,memdev=mem1 \

On X86,
-----------

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0

[AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
[AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
[AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
[AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
[AML]"NVDIMM-FIT: Rcvd buf size 000000B8"

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 

[AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
[AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
[AML]"NVDIMM-RFIT Rcvd buf size 00000004"
[AML]"NVDIMM-FIT: Rcvd buf size 00000000"
[AML]"NVDIMM-FIT: _FIT returned size 000000B8"

[ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  --> Guest receives correct size(0xb8) here 

On ARM64,
---------------

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
[Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0 

[AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
[AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
[AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
[AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
[AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 

[AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
[AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All looks same as x86 up to here.
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  ---> The size goes wrong. 8 bytes instead of 4!.
[AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
[AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
[AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes wrong. 8 bytes instead of 4!.

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size 0xb8 Dirty No.  -->Another read is attempted 
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 3  --> Error status returned

[AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
[AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
[AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"
[AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong size returned.
[ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0   -->Kernel gets 0xc0 instead of 0xb8


It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and not
sure this is a 32/64bit issue or not. I am attaching the SSDT files with the above
dbg prints added. Could you please take a look and let me know what actually is
going on here...

Much appreciated,
Shameer.

Igor Mammedov Nov. 25, 2019, 3:45 p.m. UTC | #5

On Mon, 25 Nov 2019 13:20:02 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> Hi Eric/Igor,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 22 October 2019 15:05
> > To: 'Auger Eric' <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]
> > Cc: [e-mail]; [e-mail]; xuwei (O)
> > <[e-mail]>; [e-mail]; Linuxarm
> > <[e-mail]>
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support

not related to problem discussed in this patch but you probably
need to update docs/specs/acpi_nvdimm.txt to account for your changes

> >   
> 
> [..]
> 
> > > 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...  
> 
> I spend some time debugging this further and it looks like the AML code
> behaves differently on x86 and ARM64.
FIT table is built dynamically and you are the first to debug
such issue.
(apart from the author the NVDIMM code.
 btw: why NVDIMM author is not on CC list???)


> Booted guest with nvdimm mem, and used SSDT override with dbg prints
> added,
> 
> -object memory-backend-ram,id=mem1,size=1G \
> -device nvdimm,id=dimm1,memdev=mem1 \
> 
> On X86,
> -----------
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
> [AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
> [AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
> [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
> [AML]"NVDIMM-FIT: Rcvd buf size 000000B8"
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
> [AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
> [AML]"NVDIMM-RFIT Rcvd buf size 00000004"
> [AML]"NVDIMM-FIT: Rcvd buf size 00000000"
> [AML]"NVDIMM-FIT: _FIT returned size 000000B8"
> 
> [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  --> Guest receives correct size(0xb8) here 
> 
> On ARM64,
> ---------------
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
> [Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0 
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
> [AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
> [AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
> [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
> [AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All looks same as x86 up to here.
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  ---> The size goes wrong. 8 bytes instead of 4!.

> [AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
> [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes wrong. 8 bytes instead of 4!.

                Local1 = SizeOf (Local0)
                printf("NVDIMM-RFIT Rcvd buf size %o", Local1)

                Local1 -= 0x04
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here you get -4 so sizes you see are consistent

                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o", Local1)

 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size 0xb8 Dirty No.  -->Another read is attempted 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 3  --> Error status returned

status 3 means that QEMU didn't like content of NRAM, and there is only
1 place like this in nvdimm_dsm_func_read_fit()
    if (read_fit->offset > fit->len) {                                           
        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;                         
        goto exit;                                                               
    }

so I'd start looking from here and check that QEMU gets expected data
in nvdimm_dsm_write(). In other words I'd try to trace/compare
content of DSM buffer (from qemu side).


> [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
> [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"
RFIT returns 0-sized buffer in case of error

> [AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong size returned.
after that it goes

                    Local0 = Buffer (Zero){}
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
  --> here, probably on the second iteration of the loop taking Zero
      size as EOF sign but without checking for any error (RSTA !=0)
      and returning partial buffer only

                        {
                           printf("NVDIMM-FIT: _FIT returned size %o", SizeOf(Local2))
                           Return (Local2)
                        }

relevant code in QEMU that builds this AML is in nvdimm_build_fit()

> [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0   -->Kernel gets 0xc0 instead of 0xb8
> 
> 
> It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
> ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and not
> sure this is a 32/64bit issue or not. I am attaching the SSDT files with the above
> dbg prints added. Could you please take a look and let me know what actually is
> going on here...

diff -u SSDT-dbg-x86.dsl SSDT-dbg-arm64.dsl 
--- SSDT-dbg-x86.dsl	2019-11-25 14:50:22.024983026 +0100
+++ SSDT-dbg-arm64.dsl	2019-11-25 14:50:06.740690645 +0100
@@[...]
@@ -28,7 +28,7 @@
             Method (NCAL, 5, Serialized)
             {
                 Local6 = MEMA /* \MEMA */
-                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
+                OperationRegion (NPIO, SystemMemory, 0x09090000, 0x04)
that's fine and matches your code

                 OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                 Field (NPIO, DWordAcc, NoLock, Preserve)
                 {
@@ -210,6 +210,6 @@
         }
     }
 
-    Name (MEMA, 0xBFBFD000)
+    Name (MEMA, 0xFFFF0000)

However value here is suspicious. If I recall right it should
point to DMS buffer allocated by firmware somewhere in the guest RAM.


 }
 
> Much appreciated,
> Shameer.
> 
>

Shameerali Kolothum Thodi Nov. 25, 2019, 4:25 p.m. UTC | #6

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: 25 November 2019 15:46
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Auger Eric <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; xuwei (O) <[e-mail]>;
> [e-mail]; Linuxarm <[e-mail]>
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> On Mon, 25 Nov 2019 13:20:02 +0000
> Shameerali Kolothum Thodi <[e-mail]> wrote:
> 
> > Hi Eric/Igor,
> >
> > > -----Original Message-----
> > > From: Shameerali Kolothum Thodi
> > > Sent: 22 October 2019 15:05
> > > To: 'Auger Eric' <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]
> > > Cc: [e-mail]; [e-mail]; xuwei (O)
> > > <[e-mail]>; [e-mail]; Linuxarm
> > > <[e-mail]>
> > > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> not related to problem discussed in this patch but you probably
> need to update docs/specs/acpi_nvdimm.txt to account for your changes

Ok.

> > >
> >
> > [..]
> >
> > > > 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...
> >
> > I spend some time debugging this further and it looks like the AML code
> > behaves differently on x86 and ARM64.
> FIT table is built dynamically and you are the first to debug
> such issue.
> (apart from the author the NVDIMM code.
:)
>  btw: why NVDIMM author is not on CC list???)

Right. Missed that. CCd.
 
> 
> > Booted guest with nvdimm mem, and used SSDT override with dbg prints
> > added,
> >
> > -object memory-backend-ram,id=mem1,size=1G \
> > -device nvdimm,id=dimm1,memdev=mem1 \
> >
> > On X86,
> > -----------
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8
> Dirty Yes.
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
> > [AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
> > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
> > [AML]"NVDIMM-FIT: Rcvd buf size 000000B8"
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size
> 0xb8 Dirty No.
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
> > [AML]"NVDIMM-RFIT Rcvd buf size 00000004"
> > [AML]"NVDIMM-FIT: Rcvd buf size 00000000"
> > [AML]"NVDIMM-FIT: _FIT returned size 000000B8"
> >
> > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  -->
> Guest receives correct size(0xb8) here
> >
> > On ARM64,
> > ---------------
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8
> Dirty Yes.
> > [Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
> > [AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
> > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
> > [AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size
> 0xb8 Dirty No.
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All
> looks same as x86 up to here.
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  --->
> The size goes wrong. 8 bytes instead of 4!.
> 
> > [AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
> > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes
> wrong. 8 bytes instead of 4!.
> 
>                 Local1 = SizeOf (Local0)
>                 printf("NVDIMM-RFIT Rcvd buf size %o", Local1)
> 
>                 Local1 -= 0x04
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here you get -4 so sizes you
> see are consistent

Hmm...IIUC, Method(RFIT, ) creates a buffer of size 0x4 as per 
this,

"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"

And this is the buffer received by Method(_FIT, ), 

                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("NVDIMM-FIT: Rcvd buf size %o", Local1)

But "NVDIMM-FIT: Rcvd buf size 0000000000000008".

As you can see above, in the first iteration the buffer size received by _FIT is
actually what RFIT created(0xB8).

> 
>                 If ((Local1 == Zero))
>                 {
>                     Return (Buffer (Zero){})
>                 }
> 
>                 CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
>                 printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o",
> Local1)
> 
> 
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size
> 0xb8 Dirty No.  -->Another read is attempted
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> func_ret_status 3  --> Error status returned
> 
> status 3 means that QEMU didn't like content of NRAM, and there is only
> 1 place like this in nvdimm_dsm_func_read_fit()
>     if (read_fit->offset > fit->len) {
>         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
>         goto exit;
>     }
> 
> so I'd start looking from here and check that QEMU gets expected data
> in nvdimm_dsm_write(). In other words I'd try to trace/compare
> content of DSM buffer (from qemu side).

I had printed the DSM buffer previously and it looked same, I will double check
that.
 
> 
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
> > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"
> RFIT returns 0-sized buffer in case of error

Yes.
                If ((Zero != STAU))
                {
                    Return (Buffer (Zero){})
                }
 
> > [AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong
> size returned.
> after that it goes
> 
>                     Local0 = Buffer (Zero){}
>                     Local0 = RFIT (Local3)
>                     Local1 = SizeOf (Local0)
>                     printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
>                     If ((RSTA == 0x0100))
>                     {
>                         Local2 = Buffer (Zero){}
>                         Local3 = Zero
>                     }
>                     Else
>                     {
>                         If ((Local1 == Zero))
>   --> here, probably on the second iteration of the loop taking Zero
>       size as EOF sign but without checking for any error (RSTA !=0)
>       and returning partial buffer only

I guess you meant the third iteration. Yes, it doesn't check for RSTA != 0
case. And in the second iteration, since status == 0 and buffer len received is
8 bytes, 
                        Concatenate (Local2, Local0, Local2)

The final buffer returned to Guest becomes 0xC0 (0xb8 + 0x8).
 
>                         {
>                            printf("NVDIMM-FIT: _FIT returned size %o",
> SizeOf(Local2))
>                            Return (Local2)
>                         }
> 
> relevant code in QEMU that builds this AML is in nvdimm_build_fit()
> 
> > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0
> -->Kernel gets 0xc0 instead of 0xb8
> >
> >
> > It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
> > ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and
> not
> > sure this is a 32/64bit issue or not. I am attaching the SSDT files with the
> above
> > dbg prints added. Could you please take a look and let me know what actually
> is
> > going on here...
> 
> diff -u SSDT-dbg-x86.dsl SSDT-dbg-arm64.dsl
> --- SSDT-dbg-x86.dsl	2019-11-25 14:50:22.024983026 +0100
> +++ SSDT-dbg-arm64.dsl	2019-11-25 14:50:06.740690645 +0100
> @@[...]
> @@ -28,7 +28,7 @@
>              Method (NCAL, 5, Serialized)
>              {
>                  Local6 = MEMA /* \MEMA */
> -                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
> +                OperationRegion (NPIO, SystemMemory, 0x09090000,
> 0x04)
> that's fine and matches your code
> 
>                  OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
>                  Field (NPIO, DWordAcc, NoLock, Preserve)
>                  {
> @@ -210,6 +210,6 @@
>          }
>      }
> 
> -    Name (MEMA, 0xBFBFD000)
> +    Name (MEMA, 0xFFFF0000)
> 
> However value here is suspicious. If I recall right it should
> point to DMS buffer allocated by firmware somewhere in the guest RAM.

But then it will be horribly wrong and will result in inconsistent behavior
as well, right?

Thanks,
Shameer
 
> 
>  }
> 
> > Much appreciated,
> > Shameer.
> >
> >

Igor Mammedov Nov. 26, 2019, 8:56 a.m. UTC | #7

On Mon, 25 Nov 2019 16:25:54 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:[e-mail]]
> > Sent: 25 November 2019 15:46
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: Auger Eric <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; xuwei (O) <[e-mail]>;
> > [e-mail]; Linuxarm <[e-mail]>
> > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> > 
> > On Mon, 25 Nov 2019 13:20:02 +0000
> > Shameerali Kolothum Thodi <[e-mail]> wrote:
> >   
> > > Hi Eric/Igor,
> > >  
> > > > -----Original Message-----
> > > > From: Shameerali Kolothum Thodi
> > > > Sent: 22 October 2019 15:05
> > > > To: 'Auger Eric' <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail]
> > > > Cc: [e-mail]; [e-mail]; xuwei (O)
> > > > <[e-mail]>; [e-mail]; Linuxarm
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support  
> > 
> > not related to problem discussed in this patch but you probably
> > need to update docs/specs/acpi_nvdimm.txt to account for your changes  
> 
> Ok.
> 
> > > >  
> > >
> > > [..]
> > >  
> > > > > 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...  
> > >
> > > I spend some time debugging this further and it looks like the AML code
> > > behaves differently on x86 and ARM64.  
> > FIT table is built dynamically and you are the first to debug
> > such issue.
> > (apart from the author the NVDIMM code.  
> :)
> >  btw: why NVDIMM author is not on CC list???)  
> 
> Right. Missed that. CCd.
>  
> >   
> > > Booted guest with nvdimm mem, and used SSDT override with dbg prints
> > > added,
> > >
> > > -object memory-backend-ram,id=mem1,size=1G \
> > > -device nvdimm,id=dimm1,memdev=mem1 \
> > >
> > > On X86,
> > > -----------
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8  
> > Dirty Yes.  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
> > > [AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
> > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 000000B8"
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size  
> > 0xb8 Dirty No.  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
> > > [AML]"NVDIMM-RFIT Rcvd buf size 00000004"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 00000000"
> > > [AML]"NVDIMM-FIT: _FIT returned size 000000B8"
> > >
> > > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  -->  
> > Guest receives correct size(0xb8) here  
> > >
> > > On ARM64,
> > > ---------------
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8  
> > Dirty Yes.  
> > > [Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
> > > [AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
> > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size  
> > 0xb8 Dirty No.  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All  
> > looks same as x86 up to here.  
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  --->  
> > The size goes wrong. 8 bytes instead of 4!.
> >   
> > > [AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
> > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes  
> > wrong. 8 bytes instead of 4!.
> > 
> >                 Local1 = SizeOf (Local0)
> >                 printf("NVDIMM-RFIT Rcvd buf size %o", Local1)
> > 
> >                 Local1 -= 0x04
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here you get -4 so sizes you
> > see are consistent  
> 
> Hmm...IIUC, Method(RFIT, ) creates a buffer of size 0x4 as per 
> this,
> 
> "NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> 
> And this is the buffer received by Method(_FIT, ), 
> 
>                     Local0 = RFIT (Local3)
>                     Local1 = SizeOf (Local0)
>                     printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
> 
> But "NVDIMM-FIT: Rcvd buf size 0000000000000008".
> 
> As you can see above, in the first iteration the buffer size received by _FIT is
> actually what RFIT created(0xB8).
> 
> > 
> >                 If ((Local1 == Zero))
> >                 {
> >                     Return (Buffer (Zero){})
> >                 }
> > 
> >                 CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
> >                 printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o",
> > Local1)
> > 
> >   
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size  
> > 0xb8 Dirty No.  -->Another read is attempted  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > func_ret_status 3  --> Error status returned
> > 
> > status 3 means that QEMU didn't like content of NRAM, and there is only
> > 1 place like this in nvdimm_dsm_func_read_fit()
> >     if (read_fit->offset > fit->len) {
> >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> >         goto exit;
> >     }
> > 
> > so I'd start looking from here and check that QEMU gets expected data
> > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > content of DSM buffer (from qemu side).  
> 
> I had printed the DSM buffer previously and it looked same, I will double check
> that.
>  
> >   
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"  
> > RFIT returns 0-sized buffer in case of error  
> 
> Yes.
>                 If ((Zero != STAU))
>                 {
>                     Return (Buffer (Zero){})
>                 }
>  
> > > [AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong  
> > size returned.
> > after that it goes
> > 
> >                     Local0 = Buffer (Zero){}
> >                     Local0 = RFIT (Local3)
> >                     Local1 = SizeOf (Local0)
> >                     printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
> >                     If ((RSTA == 0x0100))
> >                     {
> >                         Local2 = Buffer (Zero){}
> >                         Local3 = Zero
> >                     }
> >                     Else
> >                     {
> >                         If ((Local1 == Zero))  
> >   --> here, probably on the second iteration of the loop taking Zero  
> >       size as EOF sign but without checking for any error (RSTA !=0)
> >       and returning partial buffer only  
> 
> I guess you meant the third iteration. Yes, it doesn't check for RSTA != 0
> case. And in the second iteration, since status == 0 and buffer len received is
> 8 bytes, 
>                         Concatenate (Local2, Local0, Local2)
> 
> The final buffer returned to Guest becomes 0xC0 (0xb8 + 0x8).
>  
> >                         {
> >                            printf("NVDIMM-FIT: _FIT returned size %o",
> > SizeOf(Local2))
> >                            Return (Local2)
> >                         }
> > 
> > relevant code in QEMU that builds this AML is in nvdimm_build_fit()
> >   
> > > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0
> > -->Kernel gets 0xc0 instead of 0xb8
> > >
> > >
> > > It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
> > > ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and  
> > not  
> > > sure this is a 32/64bit issue or not. I am attaching the SSDT files with the  
> > above  
> > > dbg prints added. Could you please take a look and let me know what actually  
> > is  
> > > going on here...  
> > 
> > diff -u SSDT-dbg-x86.dsl SSDT-dbg-arm64.dsl
> > --- SSDT-dbg-x86.dsl	2019-11-25 14:50:22.024983026 +0100
> > +++ SSDT-dbg-arm64.dsl	2019-11-25 14:50:06.740690645 +0100
> > @@[...]
> > @@ -28,7 +28,7 @@
> >              Method (NCAL, 5, Serialized)
> >              {
> >                  Local6 = MEMA /* \MEMA */
> > -                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
> > +                OperationRegion (NPIO, SystemMemory, 0x09090000,
> > 0x04)
> > that's fine and matches your code
> > 
> >                  OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
> >                  Field (NPIO, DWordAcc, NoLock, Preserve)
> >                  {
> > @@ -210,6 +210,6 @@
> >          }
> >      }
> > 
> > -    Name (MEMA, 0xBFBFD000)
> > +    Name (MEMA, 0xFFFF0000)
> > 
> > However value here is suspicious. If I recall right it should
> > point to DMS buffer allocated by firmware somewhere in the guest RAM.  
> 
> But then it will be horribly wrong and will result in inconsistent behavior
> as well, right?

I'd thinks so. I'm not sure what happens but RAM starts at 0x100000000
which is above the address you have in MEMA so you shouldn't have
sensible data there and access probably should raise some sort of error.

CCing Drew

> 
> Thanks,
> Shameer
>  
> > 
> >  }
> >   
> > > Much appreciated,
> > > Shameer.
> > >
> > >  
>

Andrew Jones Nov. 26, 2019, 9:46 a.m. UTC | #8

On Tue, Nov 26, 2019 at 09:56:55AM +0100, Igor Mammedov wrote:
> On Mon, 25 Nov 2019 16:25:54 +0000
> Shameerali Kolothum Thodi <[e-mail]> wrote:
> > > -    Name (MEMA, 0xBFBFD000)
> > > +    Name (MEMA, 0xFFFF0000)
> > > 
> > > However value here is suspicious. If I recall right it should
> > > point to DMS buffer allocated by firmware somewhere in the guest RAM.  
> > 
> > But then it will be horribly wrong and will result in inconsistent behavior
> > as well, right?
> 
> I'd thinks so. I'm not sure what happens but RAM starts at 0x100000000
> which is above the address you have in MEMA so you shouldn't have
> sensible data there and access probably should raise some sort of error.
> 
> CCing Drew

RAM starts at 0x40000000 (the 1G boundary). So 0xffff0000 is OK, assuming
you want this just below the 3G boundary. With TCG, a data abort exception
will be injected for any access to an unmapped region. With KVM, unless
paging is enabled in the guest, then reads of unmapped regions will return
zero and writes will be ignored.

Thanks,
drew

Shameerali Kolothum Thodi Nov. 28, 2019, 12:36 p.m. UTC | #9

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 26 November 2019 08:57
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>; Auger Eric
> <[e-mail]>; [e-mail]; xuwei (O)
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[..]

> > > 0xb8 Dirty No.  -->Another read is attempted
> > > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> > > func_ret_status 3  --> Error status returned
> > >
> > > status 3 means that QEMU didn't like content of NRAM, and there is only
> > > 1 place like this in nvdimm_dsm_func_read_fit()
> > >     if (read_fit->offset > fit->len) {
> > >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> > >         goto exit;
> > >     }
> > >
> > > so I'd start looking from here and check that QEMU gets expected data
> > > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > > content of DSM buffer (from qemu side).
> >
> > I had printed the DSM buffer previously and it looked same, I will double check
> > that.

Tried printing the buffer in both Qemu/AML code.

On Amr64,
-------------
(1st iteration with offset 0)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0xC0
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM BUF[0x8] = 0x0
[QEMU]NVDIMM BUF[0x9] = 0x0
[QEMU]NVDIMM BUF[0xA] = 0x38
[QEMU]NVDIMM BUF[0xB] = 0x0
[QEMU]NVDIMM BUF[0xC] = 0x2
[QEMU]NVDIMM BUF[0xD] = 0x0
[QEMU]NVDIMM BUF[0xE] = 0x3
[QEMU]NVDIMM BUF[0xF] = 0x0
.........
[QEMU]NVDIMM BUF[0xBC] = 0x0
[QEMU]NVDIMM BUF[0xBD] = 0x0
[QEMU]NVDIMM BUF[0xBE] = 0x0
[QEMU]NVDIMM BUF[0xBF] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0 

"AML:NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
"AML:NVDIMM-NCAL TBUF[0000000000000000] = 0x00000000000000C0"
"AML:NVDIMM-NCAL TBUF[0000000000000001] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000002] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000003] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000004] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000005] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000006] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000007] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000008] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000009] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[000000000000000A] = 0x0000000000000038"
"AML:NVDIMM-NCAL TBUF[000000000000000B] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[000000000000000C] = 0x0000000000000002"
"AML:NVDIMM-NCAL TBUF[000000000000000D] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[000000000000000E] = 0x0000000000000003"
"AML:NVDIMM-NCAL TBUF[000000000000000F] = 0x0000000000000000"
...........
"AML:NVDIMM-NCAL TBUF[00000000000000BC] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[00000000000000BD] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[00000000000000BE] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[00000000000000BF] = 0x0000000000000000"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 00000000000000BC"
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
"AML:NVDIMM-RFIT Rcvd buf size 00000000000000BC"
"AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
"AML:NVDIMM-FIT: Rcvd buf size 00000000000000B8"     -->All looks fine in first iteration.

(2nd iteration with offset 0xb8)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 

"AML:NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000000] = 0x0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000001] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000002] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000003] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000004] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000005] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000006] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000007] = 0x0000000000000000"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 0000000000000004"   -->Requested size is 4
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008" --> Created size is 8 !.
"AML:NVDIMM-RFIT Rcvd buf size 0000000000000008"
"AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004" 
"AML:NVDIMM-FIT: Rcvd buf size 0000000000000008"  -->Again wrong here.

(3rd iteration with offset 0xc0 -->0xb8 + 0x8)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size 0xb8 Dirty No.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x3
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 3 

"AML:NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000000] = 0x0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000001] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000002] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000003] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000004] = 0x0000000000000003"
"AML:NVDIMM-NCAL TBUF[0000000000000005] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000006] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000007] = 0x0000000000000000"
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
"AML:NVDIMM-RFIT: Zero Buff return, status 0000000000000003"   -->Error status, return 
"AML:NVDIMM-FIT: Rcvd buf size 0000000000000000"
"AML:NVDIMM-FIT: _FIT returned size 00000000000000C0"
[KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57de18 sz 0xc0


On x86
(1st iteration with offset 0)
-Same as ARM64

(2nd iteration with offset 0xb8)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 
"AML:NVDIMM-NCAL: Rcvd RLEN 00000008"
"AML:NVDIMM-NCAL TBUF[00000000] = 0x00000008"
"AML:NVDIMM-NCAL TBUF[00000001] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000002] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000003] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000004] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000005] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000006] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000007] = 0x00000000"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 00000004"
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 00000004" -->Correct size
"AML:NVDIMM-RFIT Rcvd buf size 00000004"
"AML:NVDIMM-FIT: Rcvd buf size 00000000"   --> Hence there is no data payload. 
"AML:NVDIMM-FIT: _FIT returned size 000000B8"  --> No 3rd iteration. Return correct size. 
[KERNEL] acpi_nfit_init: NVDIMM: data 0xffff8c6abd5b5518 sz 0xb8

Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64, 
2nd iteration case, the created buffer size in NCAL and RFIT methods have
additional 4 bytes!.

    CreateField (ODAT, Zero, Local1, OBUF)
    Concatenate (Buffer (Zero){}, OBUF, Local7)

Please let me know if you have any clue.

Thanks,
Shameer

.

Shameerali Kolothum Thodi Dec. 9, 2019, 5:39 p.m. UTC | #10

Hi Igor/ xiaoguangrong,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 28 November 2019 12:36
> To: 'Igor Mammedov' <[e-mail]>;
> [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>; Auger Eric <[e-mail]>;
> [e-mail]; xuwei (O) <[e-mail]>;
> [e-mail]
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> 
> 
> > -----Original Message-----
> > From: Qemu-devel
> >
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 26 November 2019 08:57
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Linuxarm <[e-mail]>; Auger Eric
> > <[e-mail]>; [e-mail]; xuwei (O)
> > <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> [..]
> 
> > > > 0xb8 Dirty No.  -->Another read is attempted
> > > > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> > > > func_ret_status 3  --> Error status returned
> > > >
> > > > status 3 means that QEMU didn't like content of NRAM, and there is only
> > > > 1 place like this in nvdimm_dsm_func_read_fit()
> > > >     if (read_fit->offset > fit->len) {
> > > >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> > > >         goto exit;
> > > >     }
> > > >
> > > > so I'd start looking from here and check that QEMU gets expected data
> > > > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > > > content of DSM buffer (from qemu side).
> > >
> > > I had printed the DSM buffer previously and it looked same, I will double
> check
> > > that.
> 
> Tried printing the buffer in both Qemu/AML code.
> 
> On Amr64,

[...]
 
> Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64,
> 2nd iteration case, the created buffer size in NCAL and RFIT methods have
> additional 4 bytes!.
> 
>     CreateField (ODAT, Zero, Local1, OBUF)
>     Concatenate (Buffer (Zero){}, OBUF, Local7)
> 
> Please let me know if you have any clue.
> 

I couldn't figure out yet, why this extra 4 bytes are added by aml code on ARM64
when the nvdimm_dsm_func_read_fit() returns NvdimmFuncReadFITOut without
any FIT data. ie, when the FIT buffer len (read_len) is zero.

But the below will fix this issue,

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index f91eea3802..cddf95f4c1 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -588,7 +588,7 @@ static void nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
     nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
                  read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");

-    if (read_fit->offset > fit->len) {
+    if (read_fit->offset >= fit->len) {
         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
         goto exit;
     }


This will return error code to aml in the second iteration when there is no further
FIT data to report. But, I am not sure why this check was omitted in the first place.

Please let me know if this is acceptable and then probably I can look into a v2 of this
series.

Thanks,
Shameer

Igor Mammedov Dec. 11, 2019, 7:57 a.m. UTC | #11

On Mon, 9 Dec 2019 17:39:15 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> Hi Igor/ xiaoguangrong,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 28 November 2019 12:36
> > To: 'Igor Mammedov' <[e-mail]>;
> > [e-mail]
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Linuxarm
> > <[e-mail]>; Auger Eric <[e-mail]>;
> > [e-mail]; xuwei (O) <[e-mail]>;
> > [e-mail]
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> > 
> > 
> >   
> > > -----Original Message-----
> > > From: Qemu-devel
> > >  
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn  
> > > u.org] On Behalf Of Igor Mammedov
> > > Sent: 26 November 2019 08:57
> > > To: Shameerali Kolothum Thodi <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; Linuxarm <[e-mail]>; Auger Eric
> > > <[e-mail]>; [e-mail]; xuwei (O)
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support  
> > 
> > [..]
> >   
> > > > > 0xb8 Dirty No.  -->Another read is attempted  
> > > > > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > > > > func_ret_status 3  --> Error status returned
> > > > >
> > > > > status 3 means that QEMU didn't like content of NRAM, and there is only
> > > > > 1 place like this in nvdimm_dsm_func_read_fit()
> > > > >     if (read_fit->offset > fit->len) {
> > > > >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> > > > >         goto exit;
> > > > >     }
> > > > >
> > > > > so I'd start looking from here and check that QEMU gets expected data
> > > > > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > > > > content of DSM buffer (from qemu side).  
> > > >
> > > > I had printed the DSM buffer previously and it looked same, I will double  
> > check  
> > > > that.  
> > 
> > Tried printing the buffer in both Qemu/AML code.
> > 
> > On Amr64,  
> 
> [...]
>  
> > Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64,
> > 2nd iteration case, the created buffer size in NCAL and RFIT methods have
> > additional 4 bytes!.
> > 
> >     CreateField (ODAT, Zero, Local1, OBUF)
> >     Concatenate (Buffer (Zero){}, OBUF, Local7)
> > 
> > Please let me know if you have any clue.
> >   
> 
> I couldn't figure out yet, why this extra 4 bytes are added by aml code on ARM64
> when the nvdimm_dsm_func_read_fit() returns NvdimmFuncReadFITOut without
> any FIT data. ie, when the FIT buffer len (read_len) is zero.
> 
> But the below will fix this issue,
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index f91eea3802..cddf95f4c1 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -588,7 +588,7 @@ static void nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
>      nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
>                   read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> 
> -    if (read_fit->offset > fit->len) {
> +    if (read_fit->offset >= fit->len) {
>          func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
>          goto exit;
>      }
> 
> 
> This will return error code to aml in the second iteration when there is no further
> FIT data to report. But, I am not sure why this check was omitted in the first place.
> 
> Please let me know if this is acceptable and then probably I can look into a v2 of this
> series.
Sorry, I don't have capacity to debug this right now,
but I'd prefer if 'why' question was answered first.

Anyways, if something is unclear in how concrete AML code is build/works,
feel free to ask and I'll try to explain and guide you.

> Thanks,
> Shameer
> 
> 
>

Shameerali Kolothum Thodi Dec. 13, 2019, 12:52 p.m. UTC | #12

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: 11 December 2019 07:57
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Linuxarm <[e-mail]>; Auger Eric <[e-mail]>;
> [e-mail]; xuwei (O) <[e-mail]>;
> [e-mail]
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[...]

> > I couldn't figure out yet, why this extra 4 bytes are added by aml code on
> ARM64
> > when the nvdimm_dsm_func_read_fit() returns NvdimmFuncReadFITOut
> without
> > any FIT data. ie, when the FIT buffer len (read_len) is zero.
> >
> > But the below will fix this issue,
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index f91eea3802..cddf95f4c1 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -588,7 +588,7 @@ static void
> nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
> >      nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> >                   read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> >
> > -    if (read_fit->offset > fit->len) {
> > +    if (read_fit->offset >= fit->len) {
> >          func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> >          goto exit;
> >      }
> >
> >
> > This will return error code to aml in the second iteration when there is no
> further
> > FIT data to report. But, I am not sure why this check was omitted in the first
> place.
> >
> > Please let me know if this is acceptable and then probably I can look into a v2
> of this
> > series.
> Sorry, I don't have capacity to debug this right now,

No problem.

> but I'd prefer if 'why' question was answered first.

Right.

> Anyways, if something is unclear in how concrete AML code is build/works,
> feel free to ask and I'll try to explain and guide you.

Thanks for your help. I did spend some more time debugging this further.
I tried to introduce a totally new Buffer field object with different
sizes and printing the size after creation.

--- SSDT.dsl	2019-12-12 15:28:21.976986949 +0000
+++ SSDT-arm64-dbg.dsl	2019-12-13 12:17:11.026806186 +0000
@@ -18,7 +18,7 @@
  *     Compiler ID      "BXPC"
  *     Compiler Version 0x00000001 (1)
  */
-DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
+DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
 {
     Scope (\_SB)
     {
@@ -48,6 +48,11 @@
                     RLEN,   32, 
                     ODAT,   32736
                 }
+                
+                Field (NRAM, DWordAcc, NoLock, Preserve)
+                {
+                    NBUF,   32768 
+                }
 
                 If ((Arg4 == Zero))
                 {
@@ -87,6 +92,12 @@
                     Local3 = DerefOf (Local2)
                     FARG = Local3
                 }
+               
+                Local2 = 0x2 
+                printf("AML:NVDIMM Creating TBUF with bytes %o", Local2)
+                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
+                Concatenate (Buffer (Zero){}, TBUF, Local3)
+                printf("AML:NVDIMM Size of TBUF(Local3) %o", SizeOf(Local3))
 
                 NTFI = Local6
                 Local1 = (RLEN - 0x04)

And run it by changing Local2 with different values, It looks on ARM64, 

For cases where, Local2 <8, the created buffer size is always 8 bytes

"AML:NVDIMM Creating TBUF with bytes 0000000000000002"
"AML:NVDIMM Size of TBUF(Local3) 0000000000000008"

...
"AML:NVDIMM Creating TBUF with bytes 0000000000000005"
"AML:NVDIMM Size of TBUF(Local3) 0000000000000008"

And once Local2 >=8, it gets the correct size,

"AML:NVDIMM Creating TBUF with bytes 0000000000000009"
"AML:NVDIMM Size of TBUF(Local3) 0000000000000009"


But on x86, the behavior is like, 

For cases where, Local2 <4, the created buffer size is always 4 bytes

"AML:NVDIMM Creating TBUF with bytes 00000002"
"AML:NVDIMM Size of TBUF(Local3) 00000004"
....
"AML:NVDIMM Creating TBUF with bytes 00000003"
"AML:NVDIMM Size of TBUF(Local3) 00000004"

And once Local2 >= 4, it is ok

"AML:NVDIMM Creating TBUF with bytes 00000005"
"AML:NVDIMM Size of TBUF(Local3) 00000005"
...
"AML:NVDIMM Creating TBUF with bytes 00000009"
"AML:NVDIMM Size of TBUF(Local3) 00000009"

This is the reason why it works on x86 and not on ARM64. Because, if you
remember on second iteration of the FIT buffer, the requested buffer size is 4 .

I tried changing the AccessType of the below NBUF field from DWordAcc to
ByteAcc/BufferAcc, but no luck.

+                Field (NRAM, DWordAcc, NoLock, Preserve)
+                {
+                    NBUF,   32768 
+                }

Not sure what we need to change for ARM64 to create buffer object of size 4
here. Please let me know if you have any pointers to debug this further.

(I am attaching both x86 and ARM64 SSDT dsl used for reference)

Thanks,
Shameer


> > Thanks,
> > Shameer
> >
> >
> >

Shameerali Kolothum Thodi Jan. 6, 2020, 5:06 p.m. UTC | #13

Hi Igor,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 13 December 2019 12:52
> To: 'Igor Mammedov' <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Linuxarm <[e-mail]>; Auger Eric <[e-mail]>;
> [e-mail]; xuwei (O) <[e-mail]>;
> [e-mail]
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 

[...]

> 
> Thanks for your help. I did spend some more time debugging this further.
> I tried to introduce a totally new Buffer field object with different
> sizes and printing the size after creation.
> 
> --- SSDT.dsl	2019-12-12 15:28:21.976986949 +0000
> +++ SSDT-arm64-dbg.dsl	2019-12-13 12:17:11.026806186 +0000
> @@ -18,7 +18,7 @@
>   *     Compiler ID      "BXPC"
>   *     Compiler Version 0x00000001 (1)
>   */
> -DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
> +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
>  {
>      Scope (\_SB)
>      {
> @@ -48,6 +48,11 @@
>                      RLEN,   32,
>                      ODAT,   32736
>                  }
> +
> +                Field (NRAM, DWordAcc, NoLock, Preserve)
> +                {
> +                    NBUF,   32768
> +                }
> 
>                  If ((Arg4 == Zero))
>                  {
> @@ -87,6 +92,12 @@
>                      Local3 = DerefOf (Local2)
>                      FARG = Local3
>                  }
> +
> +                Local2 = 0x2
> +                printf("AML:NVDIMM Creating TBUF with bytes %o",
> Local2)
> +                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
> +                Concatenate (Buffer (Zero){}, TBUF, Local3)
> +                printf("AML:NVDIMM Size of TBUF(Local3) %o",
> SizeOf(Local3))
> 
>                  NTFI = Local6
>                  Local1 = (RLEN - 0x04)
> 
> And run it by changing Local2 with different values, It looks on ARM64,
> 
> For cases where, Local2 <8, the created buffer size is always 8 bytes
> 
> "AML:NVDIMM Creating TBUF with bytes 0000000000000002"
> "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> 
> ...
> "AML:NVDIMM Creating TBUF with bytes 0000000000000005"
> "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> 
> And once Local2 >=8, it gets the correct size,
> 
> "AML:NVDIMM Creating TBUF with bytes 0000000000000009"
> "AML:NVDIMM Size of TBUF(Local3) 0000000000000009"
> 
> 
> But on x86, the behavior is like,
> 
> For cases where, Local2 <4, the created buffer size is always 4 bytes
> 
> "AML:NVDIMM Creating TBUF with bytes 00000002"
> "AML:NVDIMM Size of TBUF(Local3) 00000004"
> ....
> "AML:NVDIMM Creating TBUF with bytes 00000003"
> "AML:NVDIMM Size of TBUF(Local3) 00000004"
> 
> And once Local2 >= 4, it is ok
> 
> "AML:NVDIMM Creating TBUF with bytes 00000005"
> "AML:NVDIMM Size of TBUF(Local3) 00000005"
> ...
> "AML:NVDIMM Creating TBUF with bytes 00000009"
> "AML:NVDIMM Size of TBUF(Local3) 00000009"
> 
> This is the reason why it works on x86 and not on ARM64. Because, if you
> remember on second iteration of the FIT buffer, the requested buffer size is 4 .
> 
> I tried changing the AccessType of the below NBUF field from DWordAcc to
> ByteAcc/BufferAcc, but no luck.
> 
> +                Field (NRAM, DWordAcc, NoLock, Preserve)
> +                {
> +                    NBUF,   32768
> +                }
> 
> Not sure what we need to change for ARM64 to create buffer object of size 4
> here. Please let me know if you have any pointers to debug this further.
> 
> (I am attaching both x86 and ARM64 SSDT dsl used for reference)

(+Jonathan)

Thanks to Jonathan for taking a fresh look at this issue and spotting this,
https://elixir.bootlin.com/linux/v5.5-rc5/source/drivers/acpi/acpica/utmisc.c#L109

And, from ACPI 6.3, table 19-419

"If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it
will be treated as an Integer. Otherwise, it will be treated as a Buffer. The size
of an Integer is indicated by the Definition Block table header's Revision field.
A Revision field value less than 2 indicates that the size of an Integer is 32 bits.
A value greater than or equal to 2 signifies that the size of an Integer is 64 bits."

It looks like the main reason for the difference in behavior of the buffer object
size between x86 and ARM/virt, is because of the Revision number used in the
DSDT table. On x86 it is 1 and ARM/virt it is 2.

So most likely,

>     CreateField (ODAT, Zero, Local1, OBUF)
>     Concatenate (Buffer (Zero){}, OBUF, Local7)

defaults to the minimum integer byte length based on DSDT revision number.

I tried changing the DSDT rev number of x86 code to 2,
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 614e48ff38..2941edab8d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2257,7 +2257,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
     build_header(linker, table_data,
         (void *)(table_data->data + table_data->len - dsdt->buf->len),
-        "DSDT", dsdt->buf->len, 1, NULL, NULL);
+        "DSDT", dsdt->buf->len, 2, NULL, NULL);
     free_aml_allocator();
 }

And the same issue was reported by Guest Kernel,

[    1.636672] nfit ACPI0012:00: found a zero length table '0' parsing nfit


With a quick fix to the nvdimm aml code(below) everything seems to work
now. But again this may not be the right fix/approach for this.

Please take a look and let me know.

Thanks,
Shameer

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 64eacfad08..621f9ffd41 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1192,15 +1192,18 @@ static void nvdimm_build_fit(Aml *dev)
     aml_append(method, ifctx);
 
     aml_append(method, aml_store(aml_sizeof(buf), buf_size));
-    aml_append(method, aml_subtract(buf_size,
-                                    aml_int(4) /* the size of "STAU" */,
-                                    buf_size));
 
     /* if we read the end of fit. */
-    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    ifctx = aml_if(aml_equal(aml_subtract(buf_size,
+                             aml_sizeof(aml_int(0)), NULL),
+                             aml_int(0)));
     aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
     aml_append(method, ifctx);
 
+    aml_append(method, aml_subtract(buf_size,
+                                    aml_int(4) /* the size of "STAU" */,
+                                    buf_size));
+
     aml_append(method, aml_create_field(buf,
                             aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
                             aml_shiftleft(buf_size, aml_int(3)), "BUFF"));

Igor Mammedov Jan. 9, 2020, 5:13 p.m. UTC | #14

On Mon, 6 Jan 2020 17:06:32 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 13 December 2019 12:52
> > To: 'Igor Mammedov' <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Linuxarm <[e-mail]>; Auger Eric <[e-mail]>;
> > [e-mail]; xuwei (O) <[e-mail]>;
> > [e-mail]
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> >   
> 
> [...]
> 
> > 
> > Thanks for your help. I did spend some more time debugging this further.
> > I tried to introduce a totally new Buffer field object with different
> > sizes and printing the size after creation.
> > 
> > --- SSDT.dsl	2019-12-12 15:28:21.976986949 +0000
> > +++ SSDT-arm64-dbg.dsl	2019-12-13 12:17:11.026806186 +0000
> > @@ -18,7 +18,7 @@
> >   *     Compiler ID      "BXPC"
> >   *     Compiler Version 0x00000001 (1)
> >   */
> > -DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
> > +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
> >  {
> >      Scope (\_SB)
> >      {
> > @@ -48,6 +48,11 @@
> >                      RLEN,   32,
> >                      ODAT,   32736
> >                  }
> > +
> > +                Field (NRAM, DWordAcc, NoLock, Preserve)
> > +                {
> > +                    NBUF,   32768
> > +                }
> > 
> >                  If ((Arg4 == Zero))
> >                  {
> > @@ -87,6 +92,12 @@
> >                      Local3 = DerefOf (Local2)
> >                      FARG = Local3
> >                  }
> > +
> > +                Local2 = 0x2
> > +                printf("AML:NVDIMM Creating TBUF with bytes %o",
> > Local2)
> > +                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
> > +                Concatenate (Buffer (Zero){}, TBUF, Local3)
> > +                printf("AML:NVDIMM Size of TBUF(Local3) %o",
> > SizeOf(Local3))
> > 
> >                  NTFI = Local6
> >                  Local1 = (RLEN - 0x04)
> > 
> > And run it by changing Local2 with different values, It looks on ARM64,
> > 
> > For cases where, Local2 <8, the created buffer size is always 8 bytes
> > 
> > "AML:NVDIMM Creating TBUF with bytes 0000000000000002"
> > "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> > 
> > ...
> > "AML:NVDIMM Creating TBUF with bytes 0000000000000005"
> > "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> > 
> > And once Local2 >=8, it gets the correct size,
> > 
> > "AML:NVDIMM Creating TBUF with bytes 0000000000000009"
> > "AML:NVDIMM Size of TBUF(Local3) 0000000000000009"
> > 
> > 
> > But on x86, the behavior is like,
> > 
> > For cases where, Local2 <4, the created buffer size is always 4 bytes
> > 
> > "AML:NVDIMM Creating TBUF with bytes 00000002"
> > "AML:NVDIMM Size of TBUF(Local3) 00000004"
> > ....
> > "AML:NVDIMM Creating TBUF with bytes 00000003"
> > "AML:NVDIMM Size of TBUF(Local3) 00000004"
> > 
> > And once Local2 >= 4, it is ok
> > 
> > "AML:NVDIMM Creating TBUF with bytes 00000005"
> > "AML:NVDIMM Size of TBUF(Local3) 00000005"
> > ...
> > "AML:NVDIMM Creating TBUF with bytes 00000009"
> > "AML:NVDIMM Size of TBUF(Local3) 00000009"
> > 
> > This is the reason why it works on x86 and not on ARM64. Because, if you
> > remember on second iteration of the FIT buffer, the requested buffer size is 4 .
> > 
> > I tried changing the AccessType of the below NBUF field from DWordAcc to
> > ByteAcc/BufferAcc, but no luck.
> > 
> > +                Field (NRAM, DWordAcc, NoLock, Preserve)
> > +                {
> > +                    NBUF,   32768
> > +                }
> > 
> > Not sure what we need to change for ARM64 to create buffer object of size 4
> > here. Please let me know if you have any pointers to debug this further.
> > 
> > (I am attaching both x86 and ARM64 SSDT dsl used for reference)  
> 
> (+Jonathan)
> 
> Thanks to Jonathan for taking a fresh look at this issue and spotting this,
> https://elixir.bootlin.com/linux/v5.5-rc5/source/drivers/acpi/acpica/utmisc.c#L109
> 
> And, from ACPI 6.3, table 19-419
> 
> "If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it
> will be treated as an Integer. Otherwise, it will be treated as a Buffer. The size
> of an Integer is indicated by the Definition Block table header's Revision field.
> A Revision field value less than 2 indicates that the size of an Integer is 32 bits.
> A value greater than or equal to 2 signifies that the size of an Integer is 64 bits."
> 
> It looks like the main reason for the difference in behavior of the buffer object
> size between x86 and ARM/virt, is because of the Revision number used in the
> DSDT table. On x86 it is 1 and ARM/virt it is 2.
> 
> So most likely,
> 
> >     CreateField (ODAT, Zero, Local1, OBUF)

You are right, that's where it goes wrong, since OBUF
is implicitly converted to integer if size is less than 64bits.

> >     Concatenate (Buffer (Zero){}, OBUF, Local7)

see more below

[...]

> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 64eacfad08..621f9ffd41 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1192,15 +1192,18 @@ static void nvdimm_build_fit(Aml *dev)
>      aml_append(method, ifctx);
>  
>      aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> -    aml_append(method, aml_subtract(buf_size,
> -                                    aml_int(4) /* the size of "STAU" */,
> -                                    buf_size));
>  
>      /* if we read the end of fit. */
> -    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    ifctx = aml_if(aml_equal(aml_subtract(buf_size,
> +                             aml_sizeof(aml_int(0)), NULL),
> +                             aml_int(0)));
>      aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
>      aml_append(method, ifctx);
>  
> +    aml_append(method, aml_subtract(buf_size,
> +                                    aml_int(4) /* the size of "STAU" */,
> +                                    buf_size));
> +
>      aml_append(method, aml_create_field(buf,
>                              aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
>                              aml_shiftleft(buf_size, aml_int(3)), "BUFF"));

Instead of covering up error in NCAL, I'd prefer original issue fixed.

How about something like this pseudocode:

                NTFI = Local6                                                    
                Local1 = (RLEN - 0x04)                                           
-                Local1 = (Local1 << 0x03)                                        
-                CreateField (ODAT, Zero, Local1, OBUF)                           
-                Concatenate (Buffer (Zero) {}, OBUF, Local7)   

                If (Local1 < IntegerSize)
                {
                    Local7 = Buffer(0) // output buffer
                    Local8 = 0 // index for being copied byte
                    // build byte by byte output buffer
                    while (Local8 < Local1) {
                       Local9 = Buffer(1)
                       // copy 1 byte at Local8 offset from ODAT to temporary buffer Local9
                       Store(DeRef(Index(ODAT, Local8)), Index(Local9, 0))
                       Concatenate (Local7, Local9, Local7) 
                       Increment(Local8)
                    }
                    return Local7
                } else {
                    CreateField (ODAT, Zero, Local1, OBUF)
                    return OBUF
                }

Shameerali Kolothum Thodi Jan. 13, 2020, 1:11 p.m. UTC | #15

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 09 January 2020 17:13
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Auger Eric <[e-mail]>;
> [e-mail]; Linuxarm <[e-mail]>;
> [e-mail]; [e-mail]; xuwei (O)
> <[e-mail]>; Jonathan Cameron
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> On Mon, 6 Jan 2020 17:06:32 +0000
> Shameerali Kolothum Thodi <[e-mail]> wrote:
> 
> > Hi Igor,

[...]

> > (+Jonathan)
> >
> > Thanks to Jonathan for taking a fresh look at this issue and spotting this,
> >
> https://elixir.bootlin.com/linux/v5.5-rc5/source/drivers/acpi/acpica/utmisc.c#L
> 109
> >
> > And, from ACPI 6.3, table 19-419
> >
> > "If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it
> > will be treated as an Integer. Otherwise, it will be treated as a Buffer. The
> size
> > of an Integer is indicated by the Definition Block table header's Revision field.
> > A Revision field value less than 2 indicates that the size of an Integer is 32
> bits.
> > A value greater than or equal to 2 signifies that the size of an Integer is 64
> bits."
> >
> > It looks like the main reason for the difference in behavior of the buffer object
> > size between x86 and ARM/virt, is because of the Revision number used in
> the
> > DSDT table. On x86 it is 1 and ARM/virt it is 2.
> >
> > So most likely,
> >
> > >     CreateField (ODAT, Zero, Local1, OBUF)
> 
> You are right, that's where it goes wrong, since OBUF
> is implicitly converted to integer if size is less than 64bits.
> 
> > >     Concatenate (Buffer (Zero){}, OBUF, Local7)
> 
> see more below
> 
> [...]
> 
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 64eacfad08..621f9ffd41 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -1192,15 +1192,18 @@ static void nvdimm_build_fit(Aml *dev)
> >      aml_append(method, ifctx);
> >
> >      aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> > -    aml_append(method, aml_subtract(buf_size,
> > -                                    aml_int(4) /* the size of
> "STAU" */,
> > -                                    buf_size));
> >
> >      /* if we read the end of fit. */
> > -    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> > +    ifctx = aml_if(aml_equal(aml_subtract(buf_size,
> > +                             aml_sizeof(aml_int(0)), NULL),
> > +                             aml_int(0)));
> >      aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> >      aml_append(method, ifctx);
> >
> > +    aml_append(method, aml_subtract(buf_size,
> > +                                    aml_int(4) /* the size of
> "STAU" */,
> > +                                    buf_size));
> > +
> >      aml_append(method, aml_create_field(buf,
> >                              aml_int(4 * BITS_PER_BYTE), /* offset
> at byte 4.*/
> >                              aml_shiftleft(buf_size, aml_int(3)),
> "BUFF"));
> 
> Instead of covering up error in NCAL, I'd prefer original issue fixed.
> How about something like this pseudocode:
> 
>                 NTFI = Local6
>                 Local1 = (RLEN - 0x04)
> -                Local1 = (Local1 << 0x03)
> -                CreateField (ODAT, Zero, Local1, OBUF)
> -                Concatenate (Buffer (Zero) {}, OBUF, Local7)
> 
>                 If (Local1 < IntegerSize)
>                 {
>                     Local7 = Buffer(0) // output buffer
>                     Local8 = 0 // index for being copied byte
>                     // build byte by byte output buffer
>                     while (Local8 < Local1) {
>                        Local9 = Buffer(1)
>                        // copy 1 byte at Local8 offset from ODAT to
> temporary buffer Local9
>                        Store(DeRef(Index(ODAT, Local8)), Index(Local9,
> 0))
>                        Concatenate (Local7, Local9, Local7)
>                        Increment(Local8)
>                     }
>                     return Local7
>                 } else {
>                     CreateField (ODAT, Zero, Local1, OBUF)
>                     return OBUF
>                 }
> 

Ok. This looks much better. I will test this and sent out a v2 soon addressing other
comments on this series as well.

Thanks,
Shameer