[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
|
From: |
Igor Mammedov |
|
Subject: |
Re: [PATCH 0/5] ARM virt: Add NVDIMM support |
|
Date: |
Tue, 26 Nov 2019 09:56:55 +0100 |
On Mon, 25 Nov 2019 16:25:54 +0000
Shameerali Kolothum Thodi <address@hidden> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Igor Mammedov [mailto:address@hidden]
> > Sent: 25 November 2019 15:46
> > To: Shameerali Kolothum Thodi <address@hidden>
> > Cc: Auger Eric <address@hidden>; address@hidden;
> > address@hidden; address@hidden;
> > address@hidden; xuwei (O) <address@hidden>;
> > address@hidden; Linuxarm <address@hidden>
> > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> >
> > On Mon, 25 Nov 2019 13:20:02 +0000
> > Shameerali Kolothum Thodi <address@hidden> wrote:
> >
> > > Hi Eric/Igor,
> > >
> > > > -----Original Message-----
> > > > From: Shameerali Kolothum Thodi
> > > > Sent: 22 October 2019 15:05
> > > > To: 'Auger Eric' <address@hidden>; address@hidden;
> > > > address@hidden; address@hidden
> > > > Cc: address@hidden; address@hidden; xuwei (O)
> > > > <address@hidden>; address@hidden; Linuxarm
> > > > <address@hidden>
> > > > 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.
> > >
> > >
>