On Tue, Mar 10, 2020 at 11:22:05AM +0000, Shameerali Kolothum Thodi wrote: > > > > -----Original Message----- > > From: Igor Mammedov [mailto:[email protected]] > > Sent: 06 February 2020 16:06 > > To: Shameerali Kolothum Thodi <[email protected]> > > Cc: [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; Linuxarm > > <[email protected]>; xuwei (O) <[email protected]>; > > [email protected]; [email protected] > > Subject: Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM > > output buffer length > > > > On Fri, 17 Jan 2020 17:45:17 +0000 > > Shameer Kolothum <[email protected]> wrote: > > > > > As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if the > > > Buffer Field <= to the size of an Integer (in bits), it will be > > > treated as an integer. Moreover, the integer size depends on DSDT > > > tables revision number. If revision number is < 2, integer size is 32 > > > bits, otherwise it is 64 bits. Current NVDIMM common DSM aml code > > > (NCAL) uses CreateField() for creating DSM output buffer. This creates > > > an issue in arm/virt platform where DSDT revision number is 2 and > > > results in DSM buffer with a wrong > > > size(8 bytes) gets returned when actual length is < 8 bytes. > > > This causes guest kernel to report, > > > > > > "nfit ACPI0012:00: found a zero length table '0' parsing nfit" > > > > > > In order to fix this, aml code is now modified such that it builds the > > > DSM output buffer in a byte by byte fashion when length is smaller > > > than Integer size. > > > > > > Suggested-by: Igor Mammedov <[email protected]> > > > Signed-off-by: Shameer Kolothum <[email protected]> > > > --- > > > Please find the previous discussion on this here, > > > https://patchwork.kernel.org/cover/11174959/ > > > > > > --- > > > hw/acpi/nvdimm.c | 36 > > +++++++++++++++++++-- > > > tests/qtest/bios-tables-test-allowed-diff.h | 2 ++ > > > 2 files changed, 35 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index > > > 9fdad6dc3f..5e7b8318d0 100644 > > > --- a/hw/acpi/nvdimm.c > > > +++ b/hw/acpi/nvdimm.c > > > @@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev) > > > Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, > > *elsectx2; > > > Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid; > > > Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, > > > *dsm_out_buf_size; > > > + Aml *whilectx, *offset; > > > uint8_t byte_list[1]; > > > > > > method = aml_method(NVDIMM_COMMON_DSM, 5, > > AML_SERIALIZED); @@ > > > -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev) > > > /* RLEN is not included in the payload returned to guest. */ > > > aml_append(method, > > aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE), > > > aml_int(4), dsm_out_buf_size)); > > > + > > > + /* > > > + * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if > > > + * the Buffer Field <= to the size of an Integer (in bits), it will > > > + * be treated as an integer. Moreover, the integer size depends on > > > + * DSDT tables revision number. If revision number is < 2, integer > > > + * size is 32 bits, otherwise it is 64 bits. > > > + * Because of this CreateField() canot be used if RLEN < Integer > > > Size. > > > + * Hence build dsm_out_buf byte by byte. > > > + */ > > > + ifctx = aml_if(aml_lless(dsm_out_buf_size, > > > + aml_sizeof(aml_int(0)))); > > > > this decomplies into > > > > If (Local1 < SizeOf ()) > > > > which doesn't look right > > Ok. I tried printing the value returned(SizeOf) and that looks alright.
Well it's illegal in ACPI, it's possible that OSPMs handle it the way you want them to, but it's probably not a good idea to assume they will always do. The spec says: DefSizeOf := SizeOfOp SuperName > Anyway, changed it into aml_int(1) which decompiles to > > If (Local1 < SizeOf (One)) > > Hope this is acceptable. > > Thanks, > Shameer I suspect it doesn't. And going into semantics, since they are set by ASL: 19.6.125 SizeOf (Get Data Object Size) Syntax SizeOf (ObjectName) => Integer Arguments ObjectName must be a buffer, string or package object. Description Returns the size of a buffer, string, or package data object. For a buffer, it returns the size in bytes of the data. For a string, it returns the size in bytes of the string, not counting the trailing NULL. For a package, it returns the number of elements. For an object reference, the size of the referenced object is returned. Other data types cause a fatal run-time error. Bottom line, I don't think you can figure out the integer size like this. What's wrong with just assuming 8 byte integers? I guess sizes 5 to 8 will be slower with a 32 bit DSDT but why is that a problem? -- MST
