| Message ID | 20200117174522.22044-3-shameerali.kolothum.thodi@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM virt: Add NVDIMM support | expand |
Hi Shameer, On 1/17/20 6:45 PM, Shameer Kolothum 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 <imammedo@redhat.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > 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. nit: Because of this, CreateField() cannot Thanks Eric > + * Hence build dsm_out_buf byte by byte. > + */ > + ifctx = aml_if(aml_lless(dsm_out_buf_size, aml_sizeof(aml_int(0)))); > + offset = aml_local(2); > + aml_append(ifctx, aml_store(aml_int(0), offset)); > + aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL))); > + aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf)); > + > + whilectx = aml_while(aml_lless(offset, dsm_out_buf_size)); > + /* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */ > + aml_append(whilectx, aml_store(aml_derefof(aml_index( > + aml_name(NVDIMM_DSM_OUT_BUF), offset)), > + aml_index(aml_name("TBUF"), aml_int(0)))); > + aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"), > + dsm_out_buf)); > + aml_append(whilectx, aml_increment(offset)); > + aml_append(ifctx, whilectx); > + > + aml_append(ifctx, aml_return(dsm_out_buf)); > + aml_append(method, ifctx); > + > + /* If RLEN >= Integer size, just use CreateField() operator */ > aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)), > dsm_out_buf_size)); > aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF), > aml_int(0), dsm_out_buf_size, "OBUF")); > - aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"), > - dsm_out_buf)); > - aml_append(method, aml_return(dsm_out_buf)); > + aml_append(method, aml_return(aml_name("OBUF"))); > + > aml_append(dev, method); > } > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h > index dfb8523c8b..eb8bae1407 100644 > --- a/tests/qtest/bios-tables-test-allowed-diff.h > +++ b/tests/qtest/bios-tables-test-allowed-diff.h > @@ -1 +1,3 @@ > /* List of comma-separated changed AML files to ignore */ > +"tests/data/acpi/pc/SSDT.dimmpxm", > +"tests/data/acpi/q35/SSDT.dimmpxm", >
On Fri, 17 Jan 2020 17:45:17 +0000 Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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 <imammedo@redhat.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > 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 > + offset = aml_local(2); > + aml_append(ifctx, aml_store(aml_int(0), offset)); > + aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL))); > + aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf)); > + > + whilectx = aml_while(aml_lless(offset, dsm_out_buf_size)); > + /* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */ > + aml_append(whilectx, aml_store(aml_derefof(aml_index( > + aml_name(NVDIMM_DSM_OUT_BUF), offset)), > + aml_index(aml_name("TBUF"), aml_int(0)))); > + aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"), > + dsm_out_buf)); > + aml_append(whilectx, aml_increment(offset)); > + aml_append(ifctx, whilectx); > + > + aml_append(ifctx, aml_return(dsm_out_buf)); > + aml_append(method, ifctx); > + > + /* If RLEN >= Integer size, just use CreateField() operator */ > aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)), > dsm_out_buf_size)); > aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF), > aml_int(0), dsm_out_buf_size, "OBUF")); > - aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"), > - dsm_out_buf)); > - aml_append(method, aml_return(dsm_out_buf)); > + aml_append(method, aml_return(aml_name("OBUF"))); > + > aml_append(dev, method); > } > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h > index dfb8523c8b..eb8bae1407 100644 > --- a/tests/qtest/bios-tables-test-allowed-diff.h > +++ b/tests/qtest/bios-tables-test-allowed-diff.h > @@ -1 +1,3 @@ > /* List of comma-separated changed AML files to ignore */ > +"tests/data/acpi/pc/SSDT.dimmpxm", > +"tests/data/acpi/q35/SSDT.dimmpxm",
> -----Original Message----- > From: Igor Mammedov [mailto:imammedo@redhat.com] > Sent: 06 February 2020 16:06 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > eric.auger@redhat.com; peter.maydell@linaro.org; > xiaoguangrong.eric@gmail.com; mst@redhat.com; Linuxarm > <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>; > shannon.zhaosl@gmail.com; lersek@redhat.com > 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 <shameerali.kolothum.thodi@huawei.com> 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 <imammedo@redhat.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > 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. Anyway, changed it into aml_int(1) which decompiles to If (Local1 < SizeOf (One)) Hope this is acceptable. Thanks, Shameer
On Tue, Mar 10, 2020 at 11:22:05AM +0000, Shameerali Kolothum Thodi wrote: > > > > -----Original Message----- > > From: Igor Mammedov [mailto:imammedo@redhat.com] > > Sent: 06 February 2020 16:06 > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > > eric.auger@redhat.com; peter.maydell@linaro.org; > > xiaoguangrong.eric@gmail.com; mst@redhat.com; Linuxarm > > <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>; > > shannon.zhaosl@gmail.com; lersek@redhat.com > > 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 <shameerali.kolothum.thodi@huawei.com> 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 <imammedo@redhat.com> > > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > > --- > > > 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?
> -----Original Message----- > From: Qemu-devel > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn > u.org] On Behalf Of Michael S. Tsirkin > Sent: 10 March 2020 11:36 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com; > shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm > <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org; > xuwei (O) <xuwei5@huawei.com>; Igor Mammedov > <imammedo@redhat.com>; lersek@redhat.com > Subject: Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM > output buffer length > > On Tue, Mar 10, 2020 at 11:22:05AM +0000, Shameerali Kolothum Thodi > wrote: > > > > > > > -----Original Message----- > > > From: Igor Mammedov [mailto:imammedo@redhat.com] > > > Sent: 06 February 2020 16:06 > > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > > > eric.auger@redhat.com; peter.maydell@linaro.org; > > > xiaoguangrong.eric@gmail.com; mst@redhat.com; Linuxarm > > > <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>; > > > shannon.zhaosl@gmail.com; lersek@redhat.com > > > 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 <shameerali.kolothum.thodi@huawei.com> 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 <imammedo@redhat.com> > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > > > > --- > > > > 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. Yes, I read that and was concerned. I did some experiments with SizeOf() with different integer numbers and all were returning 8. But yes, it doesn't look like the right approach. > > 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? Right. I guess that would work. I will add a comment to explain why we are using 8. Thanks, Shameer > MST >
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)))); + offset = aml_local(2); + aml_append(ifctx, aml_store(aml_int(0), offset)); + aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL))); + aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf)); + + whilectx = aml_while(aml_lless(offset, dsm_out_buf_size)); + /* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */ + aml_append(whilectx, aml_store(aml_derefof(aml_index( + aml_name(NVDIMM_DSM_OUT_BUF), offset)), + aml_index(aml_name("TBUF"), aml_int(0)))); + aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"), + dsm_out_buf)); + aml_append(whilectx, aml_increment(offset)); + aml_append(ifctx, whilectx); + + aml_append(ifctx, aml_return(dsm_out_buf)); + aml_append(method, ifctx); + + /* If RLEN >= Integer size, just use CreateField() operator */ aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)), dsm_out_buf_size)); aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF), aml_int(0), dsm_out_buf_size, "OBUF")); - aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"), - dsm_out_buf)); - aml_append(method, aml_return(dsm_out_buf)); + aml_append(method, aml_return(aml_name("OBUF"))); + aml_append(dev, method); } diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..eb8bae1407 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,3 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/SSDT.dimmpxm", +"tests/data/acpi/q35/SSDT.dimmpxm",
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 <imammedo@redhat.com> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- 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(-)