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"));
On Mon, 6 Jan 2020 17:06:32 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> 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)
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
}
> -----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 <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; drjones@redhat.com;
> xiaoguangrong.eric@gmail.com; Auger Eric <eric.auger@redhat.com>;
> qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>;
> shannon.zhaosl@gmail.com; qemu-arm@nongnu.org; xuwei (O)
> <xuwei5@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; lersek@redhat.com
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
>
> On Mon, 6 Jan 2020 17:06:32 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> 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
© 2016 - 2024 Red Hat, Inc.