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

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

> -----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