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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi Igor,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 13 December 2019 12:52
> To: 'Igor Mammedov' <imammedo@redhat.com>
> Cc: xiaoguangrong.eric@gmail.com; peter.maydell@linaro.org;
> drjones@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> Linuxarm <linuxarm@huawei.com>; Auger Eric <eric.auger@redhat.com>;
> qemu-arm@nongnu.org; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com
> 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"));