[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH 0/5] ARM virt: Add NVDIMM support
From: |
Shameerali Kolothum Thodi |
Subject: |
RE: [PATCH 0/5] ARM virt: Add NVDIMM support |
Date: |
Mon, 13 Jan 2020 13:11:53 +0000 |
> -----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 <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; Auger Eric <address@hidden>;
> address@hidden; Linuxarm <address@hidden>;
> address@hidden; address@hidden; xuwei (O)
> <address@hidden>; Jonathan Cameron
> <address@hidden>; address@hidden
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
>
> On Mon, 6 Jan 2020 17:06:32 +0000
> Shameerali Kolothum Thodi <address@hidden> 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