> -----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.th...@huawei.com> > Cc: peter.mayd...@linaro.org; drjo...@redhat.com; > xiaoguangrong.e...@gmail.com; Auger Eric <eric.au...@redhat.com>; > qemu-devel@nongnu.org; Linuxarm <linux...@huawei.com>; > shannon.zha...@gmail.com; qemu-...@nongnu.org; xuwei (O) > <xuw...@huawei.com>; Jonathan Cameron > <jonathan.came...@huawei.com>; ler...@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.th...@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
