RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> -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
> Cc: peter.mayd...@linaro.org; drjo...@redhat.com;
> xiaoguangrong.e...@gmail.com; Auger Eric ;
> qemu-devel@nongnu.org; Linuxarm ;
> shannon.zha...@gmail.com; qemu-...@nongnu.org; xuwei (O)
> ; Jonathan Cameron
> ; ler...@redhat.com
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
>
> On Mon, 6 Jan 2020 17:06:32 +
> Shameerali Kolothum Thodi 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
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
On Mon, 6 Jan 2020 17:06:32 +
Shameerali Kolothum Thodi wrote:
> Hi Igor,
>
> > -Original Message-
> > From: Shameerali Kolothum Thodi
> > Sent: 13 December 2019 12:52
> > To: 'Igor Mammedov'
> > Cc: xiaoguangrong.e...@gmail.com; peter.mayd...@linaro.org;
> > drjo...@redhat.com; shannon.zha...@gmail.com; qemu-devel@nongnu.org;
> > Linuxarm ; Auger Eric ;
> > qemu-...@nongnu.org; xuwei (O) ;
> > 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.dsl2019-12-12 15:28:21.976986949 +
> > +++ SSDT-arm64-dbg.dsl 2019-12-13 12:17:11.026806186 +
> > @@ -18,7 +18,7 @@
> > * Compiler ID "BXPC"
> > * Compiler Version 0x0001 (1)
> > */
> > -DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x0001)
> > +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x0002)
> > {
> > 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 0002"
> > "AML:NVDIMM Size of TBUF(Local3) 0008"
> >
> > ...
> > "AML:NVDIMM Creating TBUF with bytes 0005"
> > "AML:NVDIMM Size of TBUF(Local3) 0008"
> >
> > And once Local2 >=8, it gets the correct size,
> >
> > "AML:NVDIMM Creating TBUF with bytes 0009"
> > "AML:NVDIMM Size of TBUF(Local3) 0009"
> >
> >
> > 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 0002"
> > "AML:NVDIMM Size of TBUF(Local3) 0004"
> >
> > "AML:NVDIMM Creating TBUF with bytes 0003"
> > "AML:NVDIMM Size of TBUF(Local3) 0004"
> >
> > And once Local2 >= 4, it is ok
> >
> > "AML:NVDIMM Creating TBUF with bytes 0005"
> > "AML:NVDIMM Size of TBUF(Local3) 0005"
> > ...
> > "AML:NVDIMM Creating TBUF with bytes 0009"
> > "AML:NVDIMM Size of TBUF(Local3) 0009"
> >
> > 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
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Igor,
> -Original Message-
> From: Shameerali Kolothum Thodi
> Sent: 13 December 2019 12:52
> To: 'Igor Mammedov'
> Cc: xiaoguangrong.e...@gmail.com; peter.mayd...@linaro.org;
> drjo...@redhat.com; shannon.zha...@gmail.com; qemu-devel@nongnu.org;
> Linuxarm ; Auger Eric ;
> qemu-...@nongnu.org; xuwei (O) ;
> 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 +
> +++ SSDT-arm64-dbg.dsl2019-12-13 12:17:11.026806186 +
> @@ -18,7 +18,7 @@
> * Compiler ID "BXPC"
> * Compiler Version 0x0001 (1)
> */
> -DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x0001)
> +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x0002)
> {
> 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 0002"
> "AML:NVDIMM Size of TBUF(Local3) 0008"
>
> ...
> "AML:NVDIMM Creating TBUF with bytes 0005"
> "AML:NVDIMM Size of TBUF(Local3) 0008"
>
> And once Local2 >=8, it gets the correct size,
>
> "AML:NVDIMM Creating TBUF with bytes 0009"
> "AML:NVDIMM Size of TBUF(Local3) 0009"
>
>
> 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 0002"
> "AML:NVDIMM Size of TBUF(Local3) 0004"
>
> "AML:NVDIMM Creating TBUF with bytes 0003"
> "AML:NVDIMM Size of TBUF(Local3) 0004"
>
> And once Local2 >= 4, it is ok
>
> "AML:NVDIMM Creating TBUF with bytes 0005"
> "AML:NVDIMM Size of TBUF(Local3) 0005"
> ...
> "AML:NVDIMM Creating TBUF with bytes 0009"
> "AML:NVDIMM Size of TBUF(Local3) 0009"
>
> 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
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Igor,
> -Original Message-
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 11 December 2019 07:57
> To: Shameerali Kolothum Thodi
> Cc: xiaoguangrong.e...@gmail.com; peter.mayd...@linaro.org;
> drjo...@redhat.com; shannon.zha...@gmail.com; qemu-devel@nongnu.org;
> Linuxarm ; Auger Eric ;
> qemu-...@nongnu.org; xuwei (O) ;
> ler...@redhat.com
> 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.dsl2019-12-12 15:28:21.976986949 +
+++ SSDT-arm64-dbg.dsl 2019-12-13 12:17:11.026806186 +
@@ -18,7 +18,7 @@
* Compiler ID "BXPC"
* Compiler Version 0x0001 (1)
*/
-DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x0001)
+DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x0002)
{
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 0002"
"AML:NVDIMM Size of TBUF(Local3) 0008"
...
"AML:NVDIMM Creating TBUF with bytes 0005"
"AML:NVDIMM Size of TBUF(Local3) 0008"
And once Local2 >=8, it gets the correct size,
"AML:NVDIMM Creating TBUF with bytes 0009"
"AML:NVDIMM Size of TBUF(Local3) 0009"
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 0002"
"AML:NVDIMM Size of TBUF(Local3) 0004"
"AML:NVDIMM Creating TBUF with bytes 0003"
"AML:NVDIMM Size of TBUF(Local3) 0004"
And once Local2 >= 4, it is ok
"AML:NVDIMM Creating TBUF with bytes 0005"
"AML:NVDIMM Size of TBUF(Local3) 0005"
...
"AML:NVDIMM Creating TBUF with bytes 0009"
"AML:NVDIMM Size of TBUF(Local3) 0009"
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 bu
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
On Mon, 9 Dec 2019 17:39:15 +
Shameerali Kolothum Thodi wrote:
> Hi Igor/ xiaoguangrong,
>
> > -Original Message-
> > From: Shameerali Kolothum Thodi
> > Sent: 28 November 2019 12:36
> > To: 'Igor Mammedov' ;
> > xiaoguangrong.e...@gmail.com
> > Cc: peter.mayd...@linaro.org; drjo...@redhat.com;
> > shannon.zha...@gmail.com; qemu-devel@nongnu.org; Linuxarm
> > ; Auger Eric ;
> > qemu-...@nongnu.org; xuwei (O) ;
> > ler...@redhat.com
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> >
> >
> >
> > > -Original Message-
> > > From: Qemu-devel
> > >
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > > u.org] On Behalf Of Igor Mammedov
> > > Sent: 26 November 2019 08:57
> > > To: Shameerali Kolothum Thodi
> > > Cc: peter.mayd...@linaro.org; drjo...@redhat.com;
> > > xiaoguangrong.e...@gmail.com; shannon.zha...@gmail.com;
> > > qemu-devel@nongnu.org; Linuxarm ; Auger Eric
> > > ; qemu-...@nongnu.org; xuwei (O)
> > > ; ler...@redhat.com
> > > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> >
> > [..]
> >
> > > > > 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.
> >
> > Tried printing the buffer in both Qemu/AML code.
> >
> > On Amr64,
>
> [...]
>
> > Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64,
> > 2nd iteration case, the created buffer size in NCAL and RFIT methods have
> > additional 4 bytes!.
> >
> > CreateField (ODAT, Zero, Local1, OBUF)
> > Concatenate (Buffer (Zero){}, OBUF, Local7)
> >
> > Please let me know if you have any clue.
> >
>
> 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,
but I'd prefer if 'why' question was answered first.
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,
> Shameer
>
>
>
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Igor/ xiaoguangrong,
> -Original Message-
> From: Shameerali Kolothum Thodi
> Sent: 28 November 2019 12:36
> To: 'Igor Mammedov' ;
> xiaoguangrong.e...@gmail.com
> Cc: peter.mayd...@linaro.org; drjo...@redhat.com;
> shannon.zha...@gmail.com; qemu-devel@nongnu.org; Linuxarm
> ; Auger Eric ;
> qemu-...@nongnu.org; xuwei (O) ;
> ler...@redhat.com
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
>
>
>
> > -Original Message-
> > From: Qemu-devel
> >
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 26 November 2019 08:57
> > To: Shameerali Kolothum Thodi
> > Cc: peter.mayd...@linaro.org; drjo...@redhat.com;
> > xiaoguangrong.e...@gmail.com; shannon.zha...@gmail.com;
> > qemu-devel@nongnu.org; Linuxarm ; Auger Eric
> > ; qemu-...@nongnu.org; xuwei (O)
> > ; ler...@redhat.com
> > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
>
> [..]
>
> > > > 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.
>
> Tried printing the buffer in both Qemu/AML code.
>
> On Amr64,
[...]
> Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64,
> 2nd iteration case, the created buffer size in NCAL and RFIT methods have
> additional 4 bytes!.
>
> CreateField (ODAT, Zero, Local1, OBUF)
> Concatenate (Buffer (Zero){}, OBUF, Local7)
>
> Please let me know if you have any clue.
>
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.
Thanks,
Shameer
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 26 November 2019 08:57
> To: Shameerali Kolothum Thodi
> Cc: peter.mayd...@linaro.org; drjo...@redhat.com;
> xiaoguangrong.e...@gmail.com; shannon.zha...@gmail.com;
> qemu-devel@nongnu.org; Linuxarm ; Auger Eric
> ; qemu-...@nongnu.org; xuwei (O)
> ; ler...@redhat.com
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
[..]
> > > 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.
Tried printing the buffer in both Qemu/AML code.
On Amr64,
-
(1st iteration with offset 0)
[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 buff:
[QEMU]NVDIMM BUF[0x0] = 0xC0
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM BUF[0x8] = 0x0
[QEMU]NVDIMM BUF[0x9] = 0x0
[QEMU]NVDIMM BUF[0xA] = 0x38
[QEMU]NVDIMM BUF[0xB] = 0x0
[QEMU]NVDIMM BUF[0xC] = 0x2
[QEMU]NVDIMM BUF[0xD] = 0x0
[QEMU]NVDIMM BUF[0xE] = 0x3
[QEMU]NVDIMM BUF[0xF] = 0x0
.
[QEMU]NVDIMM BUF[0xBC] = 0x0
[QEMU]NVDIMM BUF[0xBD] = 0x0
[QEMU]NVDIMM BUF[0xBE] = 0x0
[QEMU]NVDIMM BUF[0xBF] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0
func_ret_status 0
"AML:NVDIMM-NCAL: Rcvd RLEN 00C0"
"AML:NVDIMM-NCAL TBUF[] = 0x00C0"
"AML:NVDIMM-NCAL TBUF[0001] = 0x"
"AML:NVDIMM-NCAL TBUF[0002] = 0x"
"AML:NVDIMM-NCAL TBUF[0003] = 0x"
"AML:NVDIMM-NCAL TBUF[0004] = 0x"
"AML:NVDIMM-NCAL TBUF[0005] = 0x"
"AML:NVDIMM-NCAL TBUF[0006] = 0x"
"AML:NVDIMM-NCAL TBUF[0007] = 0x"
"AML:NVDIMM-NCAL TBUF[0008] = 0x"
"AML:NVDIMM-NCAL TBUF[0009] = 0x"
"AML:NVDIMM-NCAL TBUF[000A] = 0x0038"
"AML:NVDIMM-NCAL TBUF[000B] = 0x"
"AML:NVDIMM-NCAL TBUF[000C] = 0x0002"
"AML:NVDIMM-NCAL TBUF[000D] = 0x"
"AML:NVDIMM-NCAL TBUF[000E] = 0x0003"
"AML:NVDIMM-NCAL TBUF[000F] = 0x"
...
"AML:NVDIMM-NCAL TBUF[00BC] = 0x"
"AML:NVDIMM-NCAL TBUF[00BD] = 0x"
"AML:NVDIMM-NCAL TBUF[00BE] = 0x"
"AML:NVDIMM-NCAL TBUF[00BF] = 0x"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 00BC"
"AML:NVDIMM-NCAL: Created BUF(Local7) size 00BC"
"AML:NVDIMM-RFIT Rcvd buf size 00BC"
"AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8"
"AML:NVDIMM-FIT: Rcvd buf size 00B8" -->All looks fine in first
iteration.
(2nd iteration with offset 0xb8)
[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 buff:
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
func_ret_status 0
"AML:NVDIMM-NCAL: Rcvd RLEN 0008"
"AML:NVDIMM-NCAL TBUF[] = 0x0008"
"AML:NVDIMM-NCAL TBUF[0001] = 0x"
"AML:NVDIMM-NCAL TBUF[0002] =
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
On Tue, Nov 26, 2019 at 09:56:55AM +0100, Igor Mammedov wrote: > On Mon, 25 Nov 2019 16:25:54 + > Shameerali Kolothum Thodi wrote: > > > -Name (MEMA, 0xBFBFD000) > > > +Name (MEMA, 0x) > > > > > > 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 0x1 > 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 RAM starts at 0x4000 (the 1G boundary). So 0x is OK, assuming you want this just below the 3G boundary. With TCG, a data abort exception will be injected for any access to an unmapped region. With KVM, unless paging is enabled in the guest, then reads of unmapped regions will return zero and writes will be ignored. Thanks, drew
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
On Mon, 25 Nov 2019 16:25:54 + Shameerali Kolothum Thodi wrote: > Hi Igor, > > > -Original Message- > > From: Igor Mammedov [mailto:imamm...@redhat.com] > > Sent: 25 November 2019 15:46 > > To: Shameerali Kolothum Thodi > > Cc: Auger Eric ; qemu-devel@nongnu.org; > > qemu-...@nongnu.org; peter.mayd...@linaro.org; > > shannon.zha...@gmail.com; xuwei (O) ; > > ler...@redhat.com; Linuxarm > > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support > > > > On Mon, 25 Nov 2019 13:20:02 + > > Shameerali Kolothum Thodi wrote: > > > > > Hi Eric/Igor, > > > > > > > -Original Message- > > > > From: Shameerali Kolothum Thodi > > > > Sent: 22 October 2019 15:05 > > > > To: 'Auger Eric' ; qemu-devel@nongnu.org; > > > > qemu-...@nongnu.org; imamm...@redhat.com > > > > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; xuwei (O) > > > > ; ler...@redhat.com; Linuxarm > > > > > > > > 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 0x8a273dc12b18 sz 0xb8 > > > > > > > > ARM64 Guest, > > > > [0.933133] acpi_nfit_init: data 0x3cbe6018 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 00C0" > > > [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" > > > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 00BC" > > > [AML]"NVDIMM-RFIT Rcvd buf size 00BC" > > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8" > > > [AML]"NVDIMM-FIT: Rcvd buf size 00B8" > > > > > > [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 0008" > > > [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" > > > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0004" > > > [AML]"NVDIMM-RFIT Rcvd buf size 0004" > > > [AML]"NVDIMM-FIT: Rcvd buf size " > >
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Igor, > -Original Message- > From: Igor Mammedov [mailto:imamm...@redhat.com] > Sent: 25 November 2019 15:46 > To: Shameerali Kolothum Thodi > Cc: Auger Eric ; qemu-devel@nongnu.org; > qemu-...@nongnu.org; peter.mayd...@linaro.org; > shannon.zha...@gmail.com; xuwei (O) ; > ler...@redhat.com; Linuxarm > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support > > On Mon, 25 Nov 2019 13:20:02 + > Shameerali Kolothum Thodi wrote: > > > Hi Eric/Igor, > > > > > -Original Message- > > > From: Shameerali Kolothum Thodi > > > Sent: 22 October 2019 15:05 > > > To: 'Auger Eric' ; qemu-devel@nongnu.org; > > > qemu-...@nongnu.org; imamm...@redhat.com > > > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; xuwei (O) > > > ; ler...@redhat.com; Linuxarm > > > > > > 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 0x8a273dc12b18 sz 0xb8 > > > > > > ARM64 Guest, > > > [0.933133] acpi_nfit_init: data 0x3cbe6018 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 00C0" > > [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" > > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 00BC" > > [AML]"NVDIMM-RFIT Rcvd buf size 00BC" > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8" > > [AML]"NVDIMM-FIT: Rcvd buf size 00B8" > > > > [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 0008" > > [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" > > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0004" > > [AML]"NVDIMM-RFIT Rcvd buf size 0004" > > [AML]"NVDIMM-FIT: Rcvd buf size " > > [AML]"NVDIMM-FIT: _FIT returned size 00B8" > > > > [ KERNEL] acpi_nfit_init: NVDIMM: data 0x9855bb9a7518 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 00C0" > > [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" > > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
On Mon, 25 Nov 2019 13:20:02 + Shameerali Kolothum Thodi wrote: > Hi Eric/Igor, > > > -Original Message- > > From: Shameerali Kolothum Thodi > > Sent: 22 October 2019 15:05 > > To: 'Auger Eric' ; qemu-devel@nongnu.org; > > qemu-...@nongnu.org; imamm...@redhat.com > > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; xuwei (O) > > ; ler...@redhat.com; Linuxarm > > > > 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 > > > > [..] > > > > 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 0x8a273dc12b18 sz 0xb8 > > > > ARM64 Guest, > > [0.933133] acpi_nfit_init: data 0x3cbe6018 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???) > 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 00C0" > [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 00BC" > [AML]"NVDIMM-RFIT Rcvd buf size 00BC" > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8" > [AML]"NVDIMM-FIT: Rcvd buf size 00B8" > > [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 0008" > [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0004" > [AML]"NVDIMM-RFIT Rcvd buf size 0004" > [AML]"NVDIMM-FIT: Rcvd buf size " > [AML]"NVDIMM-FIT: _FIT returned size 00B8" > > [ KERNEL] acpi_nfit_init: NVDIMM: data 0x9855bb9a7518 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 00C0" > [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 00BC" > [AML]"NVDIMM-RFIT Rcvd buf size 00BC" > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8" > [AML]"NVDIMM-FIT: Rcvd buf size 00B8" > > [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 0008" > [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" --> All looks > same as x86 up to here. > [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0008" ---> The size > goes wrong. 8 bytes instead of 4!. > [AML]"NVDIMM-RFIT Rcvd buf size 0008" > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Eric/Igor, > -Original Message- > From: Shameerali Kolothum Thodi > Sent: 22 October 2019 15:05 > To: 'Auger Eric' ; qemu-devel@nongnu.org; > qemu-...@nongnu.org; imamm...@redhat.com > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; xuwei (O) > ; ler...@redhat.com; Linuxarm > > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support > [..] > > 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 0x8a273dc12b18 sz 0xb8 > > ARM64 Guest, > [0.933133] acpi_nfit_init: data 0x3cbe6018 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. 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 00C0" [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" [AML]"NVDIMM-NCAL: Created BUF(Local7) size 00BC" [AML]"NVDIMM-RFIT Rcvd buf size 00BC" [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8" [AML]"NVDIMM-FIT: Rcvd buf size 00B8" [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 0008" [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0004" [AML]"NVDIMM-RFIT Rcvd buf size 0004" [AML]"NVDIMM-FIT: Rcvd buf size " [AML]"NVDIMM-FIT: _FIT returned size 00B8" [ KERNEL] acpi_nfit_init: NVDIMM: data 0x9855bb9a7518 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 00C0" [AML]"NVDIMM-NCAL: Creating OBUF with 05E0 bits" [AML]"NVDIMM-NCAL: Created BUF(Local7) size 00BC" [AML]"NVDIMM-RFIT Rcvd buf size 00BC" [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00B8" [AML]"NVDIMM-FIT: Rcvd buf size 00B8" [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 0008" [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" --> All looks same as x86 up to here. [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0008" ---> The size goes wrong. 8 bytes instead of 4!. [AML]"NVDIMM-RFIT Rcvd buf size 0008" [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0004" [AML]"NVDIMM-FIT: Rcvd buf size 0008" --> Again size goes wrong. 8 bytes instead of 4!. [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 [AML]"NVDIMM-NCAL: Rcvd RLEN 0008" [AML]"NVDIMM-NCAL: Creating OBUF with 0020 bits" [AML]"NVDIMM-NCAL: Created BUF(Local7) size 0008" [AML]"NVDIMM-FIT: Rcvd buf size " [AML]"NVDIMM-FIT: _FIT returned size 00C0" --> Wrong size returned. [ KERNEL] acpi_nfit_init: NVDIMM: data 0xfc57ce18 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... Much appreciated, Shameer. SSDT-dbg-arm64.dsl Description: SSDT-dbg-arm64.dsl SSDT-dbg-x86.dsl Description: SSDT-dbg-x86.dsl
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
On Fri, 4 Oct 2019 16:52:57 +0100 Shameer Kolothum wrote: > This series adds NVDIMM support to arm/virt platform. > This has a dependency on [0] and make use of the GED > device for NVDIMM hotplug events. The series reuses > some of the patches posted by Eric in his earlier > attempt here[1]. > > Patch 1/5 is a fix to the Guest reboot issue on NVDIMM > hot add case described here[2]. > > I have done basic sanity testing of NVDIMM deviecs with > both ACPI and DT Guest boot. Further testing is always > welcome. > > Please let me know your feedback. one more thing, It's possible to run bios-tables-test for virt/arm now, pls add corresponding test case it might be easier to just extend test_acpi_virt_tcg_memhp() with nvdimms > > Thanks, > Shameer > > [0] https://patchwork.kernel.org/cover/11150345/ > [1] https://patchwork.kernel.org/cover/10830777/ > [2] https://patchwork.kernel.org/patch/11154757/ > > Eric Auger (1): > hw/arm/boot: Expose the pmem nodes in the DT > > Kwangwoo Lee (2): > nvdimm: Use configurable ACPI IO base and size > hw/arm/virt: Add nvdimm hot-plug infrastructure > > Shameer Kolothum (2): > hw/arm: Align ACPI blob len to PAGE size > hw/arm/virt: Add nvdimm hotplug support > > docs/specs/acpi_hw_reduced_hotplug.rst | 1 + > hw/acpi/generic_event_device.c | 13 > hw/acpi/nvdimm.c | 32 -- > hw/arm/Kconfig | 1 + > hw/arm/boot.c | 45 ++ > hw/arm/virt-acpi-build.c | 20 > hw/arm/virt.c | 42 > hw/i386/acpi-build.c | 6 > hw/i386/acpi-build.h | 3 ++ > hw/i386/pc_piix.c | 2 ++ > hw/i386/pc_q35.c | 2 ++ > hw/mem/Kconfig | 2 +- > include/hw/acpi/generic_event_device.h | 1 + > include/hw/arm/virt.h | 1 + > include/hw/mem/nvdimm.h| 3 ++ > 15 files changed, 157 insertions(+), 17 deletions(-) >
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Eric,
> -Original Message-
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: 18 October 2019 17:40
> To: Shameerali Kolothum Thodi ;
> qemu-devel@nongnu.org; qemu-...@nongnu.org; imamm...@redhat.com
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; xuwei (O)
> ; ler...@redhat.com; Linuxarm
>
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
>
> Hi Shameer,
>
> On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> > This series adds NVDIMM support to arm/virt platform.
> > This has a dependency on [0] and make use of the GED
> > device for NVDIMM hotplug events. The series reuses
> > some of the patches posted by Eric in his earlier
> > attempt here[1].
> >
> > Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> > hot add case described here[2].
> >
> > I have done basic sanity testing of NVDIMM deviecs with
> devcies
> > both ACPI and DT Guest boot. Further testing is always
> > welcome.
> >
> > Please let me know your feedback.
>
> I tested it on my side. Looks to work pretty well.
Thanks for giving this a spin.
> 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 0x8a273dc12b18 sz 0xb8
ARM64 Guest,
[0.933133] acpi_nfit_init: data 0x3cbe6018 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...
> Besides when we reset the system we find the namespaces again using
> "ndctl list -u" so the original bug seems to be fixed.
>
> Did you try to mount a DAX FS. I can mount but with DAX forced off.
> sudo mkdir /mnt/mem0
Yes. I did try with DAX FS. But do we need to change the namespace mode to
file system DAX mode?
I used the below command before attempting to mount with -o dax,
ndctl create-namespace -f -e namespace0.0 --mode=fsdax
And in order to do the above you might need the ZONE_DEVICE support
in the Kernel which in turn has dependency on hot remove. Hence I tried with
"arm64/mm: Enable memory hot remove" patches,
https://patchwork.kernel.org/cover/11185169/
> mkfs.xfs -f -m reflink=0 /dev/pmem0
> sudo mount -o dax /dev/pmem0 /mnt/mem0
> [ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
> your own risk
> [ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
> DAX.
> [ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
> [ 2610.189797] XFS (pmem0): Ending clean mount
>
> I fail to remember if it was the case months ago. I am not sure if it is
> an issue in my guest .config or if there is something not yet supported
> on aarch64? Did you try on your side?
>
> Also if you forget to put the ",nvdimm" to the machvirt options you get,
> on hotplug:
> {"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
> which is not correct anymore ;-)
Ok. I will check this.
Thanks,
Shameer
> Thanks
>
> Eric
>
>
> >
> > Thanks,
> > Shameer
> >
> > [0] https://patchwork.kernel.org/cover/11150345/
> > [1] https://patchwork.kernel.org/cover/10830777/
> > [2] https://patchwork.kernel.org/patch/11154757/
> >
> > Eric Auger (1):
> > hw/arm/boot: Expose the pmem nodes in the DT
> >
> > Kwangwoo Lee (2):
> > nvdimm: Use configurable ACPI IO base and size
> > hw/arm/virt: Add nvdimm hot-plug infrastructure
> >
> > Shameer Kolothum (2):
> > hw/arm: Align ACPI blob len to PAGE size
> > hw/arm/virt: Add nvdimm hotplug support
> >
> > docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
> > hw/acpi/generic_event_device.c | 13
> > hw/acpi/nvdimm.c | 32 --
> > hw/arm/Kconfig | 1 +
> > hw/arm/boot.c | 45
> ++
> > hw/arm/virt-acpi-build.c | 20
> > hw/arm/virt.c | 42
>
> > hw/i386/acpi-build.c | 6
> > hw/i386/acpi-build.h | 3 ++
> > hw/i386/pc_piix.c | 2 ++
> > hw/i386/pc_q35.c | 2 ++
> > hw/mem/Kconfig | 2 +-
> > include/hw/acpi/generic_event_device.h | 1 +
> > include/hw/arm/virt.h | 1 +
> > include/hw/mem/nvdimm.h| 3 ++
> > 15 files changed, 157 insertions(+), 17 deletions(-)
> >
Re: [PATCH 0/5] ARM virt: Add NVDIMM support
Hi Shameer,
On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> This series adds NVDIMM support to arm/virt platform.
> This has a dependency on [0] and make use of the GED
> device for NVDIMM hotplug events. The series reuses
> some of the patches posted by Eric in his earlier
> attempt here[1].
>
> Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> hot add case described here[2].
>
> I have done basic sanity testing of NVDIMM deviecs with
devcies
> both ACPI and DT Guest boot. Further testing is always
> welcome.
>
> Please let me know your feedback.
I tested it on my side. Looks to work pretty well.
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?
Besides when we reset the system we find the namespaces again using
"ndctl list -u" so the original bug seems to be fixed.
Did you try to mount a DAX FS. I can mount but with DAX forced off.
sudo mkdir /mnt/mem0
mkfs.xfs -f -m reflink=0 /dev/pmem0
sudo mount -o dax /dev/pmem0 /mnt/mem0
[ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
your own risk
[ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
DAX.
[ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
[ 2610.189797] XFS (pmem0): Ending clean mount
I fail to remember if it was the case months ago. I am not sure if it is
an issue in my guest .config or if there is something not yet supported
on aarch64? Did you try on your side?
Also if you forget to put the ",nvdimm" to the machvirt options you get,
on hotplug:
{"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
which is not correct anymore ;-)
Thanks
Eric
>
> Thanks,
> Shameer
>
> [0] https://patchwork.kernel.org/cover/11150345/
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/patch/11154757/
>
> Eric Auger (1):
> hw/arm/boot: Expose the pmem nodes in the DT
>
> Kwangwoo Lee (2):
> nvdimm: Use configurable ACPI IO base and size
> hw/arm/virt: Add nvdimm hot-plug infrastructure
>
> Shameer Kolothum (2):
> hw/arm: Align ACPI blob len to PAGE size
> hw/arm/virt: Add nvdimm hotplug support
>
> docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
> hw/acpi/generic_event_device.c | 13
> hw/acpi/nvdimm.c | 32 --
> hw/arm/Kconfig | 1 +
> hw/arm/boot.c | 45 ++
> hw/arm/virt-acpi-build.c | 20
> hw/arm/virt.c | 42
> hw/i386/acpi-build.c | 6
> hw/i386/acpi-build.h | 3 ++
> hw/i386/pc_piix.c | 2 ++
> hw/i386/pc_q35.c | 2 ++
> hw/mem/Kconfig | 2 +-
> include/hw/acpi/generic_event_device.h | 1 +
> include/hw/arm/virt.h | 1 +
> include/hw/mem/nvdimm.h| 3 ++
> 15 files changed, 157 insertions(+), 17 deletions(-)
>
[PATCH 0/5] ARM virt: Add NVDIMM support
This series adds NVDIMM support to arm/virt platform. This has a dependency on [0] and make use of the GED device for NVDIMM hotplug events. The series reuses some of the patches posted by Eric in his earlier attempt here[1]. Patch 1/5 is a fix to the Guest reboot issue on NVDIMM hot add case described here[2]. I have done basic sanity testing of NVDIMM deviecs with both ACPI and DT Guest boot. Further testing is always welcome. Please let me know your feedback. Thanks, Shameer [0] https://patchwork.kernel.org/cover/11150345/ [1] https://patchwork.kernel.org/cover/10830777/ [2] https://patchwork.kernel.org/patch/11154757/ Eric Auger (1): hw/arm/boot: Expose the pmem nodes in the DT Kwangwoo Lee (2): nvdimm: Use configurable ACPI IO base and size hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum (2): hw/arm: Align ACPI blob len to PAGE size hw/arm/virt: Add nvdimm hotplug support docs/specs/acpi_hw_reduced_hotplug.rst | 1 + hw/acpi/generic_event_device.c | 13 hw/acpi/nvdimm.c | 32 -- hw/arm/Kconfig | 1 + hw/arm/boot.c | 45 ++ hw/arm/virt-acpi-build.c | 20 hw/arm/virt.c | 42 hw/i386/acpi-build.c | 6 hw/i386/acpi-build.h | 3 ++ hw/i386/pc_piix.c | 2 ++ hw/i386/pc_q35.c | 2 ++ hw/mem/Kconfig | 2 +- include/hw/acpi/generic_event_device.h | 1 + include/hw/arm/virt.h | 1 + include/hw/mem/nvdimm.h| 3 ++ 15 files changed, 157 insertions(+), 17 deletions(-) -- 2.17.1
