RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden]
> Sent: 11 December 2019 07:57
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> Linuxarm <address@hidden>; Auger Eric <address@hidden>;
> address@hidden; xuwei (O) <address@hidden>;
> address@hidden
> 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
> >
> >
> >

SSDT-arm64-dbg.dsl
Description: SSDT-arm64-dbg.dsl

SSDT-x86-dbg.dsl
Description: SSDT-x86-dbg.dsl