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

[Igor Mammedov <imammedo@redhat.com>]

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
                }