On Mon, 6 Jan 2020 17:06:32 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 13 December 2019 12:52
> > To: 'Igor Mammedov' <imamm...@redhat.com>
> > Cc: xiaoguangrong.e...@gmail.com; peter.mayd...@linaro.org;
> > drjo...@redhat.com; shannon.zha...@gmail.com; qemu-devel@nongnu.org;
> > Linuxarm <linux...@huawei.com>; Auger Eric <eric.au...@redhat.com>;
> > qemu-...@nongnu.org; xuwei (O) <xuw...@huawei.com>;
> > ler...@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
}
