[PATCH 0/4] acpi: fix short OEM [Table] ID padding

[PATCH 0/4] acpi: fix short OEM [Table] ID padding

[Igor Mammedov]

Since 6.0 the commit:
  602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
regressed values of OEM [Table] ID fields in ACPI tables
by padding them with whitespace is a value is shorter then
max possible. That depending on vendor broke OEM [Table] ID patching
with SLIC table values and as result licensing of Windows guests.

First reported here https://gitlab.com/qemu-project/qemu/-/issues/707

CC: Marian Postevca <posteuca@mutex.one>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Ani Sinha <ani@anisinha.ca>

Igor Mammedov (4):
  tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields()
    test
  tests: acpi: whitelist nvdimm's SSDT and FACP.slic expected blobs
  acpi: fix OEM ID/OEM Table ID padding
  tests: acpi: update expected blobs

 hw/acpi/aml-build.c              |   4 ++--
 tests/data/acpi/pc/SSDT.dimmpxm  | Bin 734 -> 734 bytes
 tests/data/acpi/q35/FACP.slic    | Bin 244 -> 244 bytes
 tests/data/acpi/q35/SSDT.dimmpxm | Bin 734 -> 734 bytes
 tests/data/acpi/virt/SSDT.memhp  | Bin 736 -> 736 bytes
 tests/qtest/bios-tables-test.c   |  15 ++++++---------
 6 files changed, 8 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH 1/4] tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields() test

[Igor Mammedov]

The next commit will revert OEM fields padding with whitespace to
padding with '\0' as it was before [1]. As result test_oem_fields() will
fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables.

Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test
puts on QEMU CLI and expected values match.

1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index e6b72d9026..90c9f6a0a2 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -71,9 +71,10 @@
 
 #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
 
-#define OEM_ID             "TEST"
-#define OEM_TABLE_ID       "OEM"
-#define OEM_TEST_ARGS      "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID
+#define OEM_ID             "TEST  "
+#define OEM_TABLE_ID       "OEM     "
+#define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
+                           OEM_TABLE_ID "'"
 
 typedef struct {
     bool tcg_only;
@@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void)
 static void test_oem_fields(test_data *data)
 {
     int i;
-    char oem_id[6];
-    char oem_table_id[8];
 
-    strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' ');
-    strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' ');
     for (i = 0; i < data->tables->len; ++i) {
         AcpiSdtTable *sdt;
 
@@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data)
             continue;
         }
 
-        g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0);
-        g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0);
+        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
+        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
     }
 }
 
-- 
2.31.1

[PATCH 2/4] tests: acpi: whitelist nvdimm's SSDT and FACP.slic expected blobs

[Igor Mammedov]

The next commit will revert OEM fields whitespace padding to
padding with '\0' as it was before [1]. That will change OEM
Table ID for:
  * SSDT.*: where it was padded from 6 characters to 8
  * FACP.slic: where it was padded from 2 characters to 8
after reverting whitespace padding, it will be replaced with
'\0' which effectively will shorten OEM table ID to 6 and 2
characters.

Whitelist affected tables before introducing the change.

1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..7faa8f53be 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,5 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/SSDT.memhp",
+"tests/data/acpi/pc/SSDT.dimmpxm",
+"tests/data/acpi/q35/SSDT.dimmpxm",
+"tests/data/acpi/q35/FACP.slic",
-- 
2.31.1

[PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Igor Mammedov]

Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
fields in headers of ACPI tables. While it doesn't have impact on
default values since QEMU uses 6 and 8 characters long values
respectively, it broke usecase where IDs are provided on QEMU CLI.
It shouldn't affect guest (but may cause licensing verification
issues in guest OS).
One of the broken usecases is user supplied SLIC table with IDs
shorter than max possible length, where [2] mangles IDs with extra
spaces in RSDT and FADT tables whereas guest OS expects those to
mirror the respective values of the used SLIC table.

Fix it by replacing whitespace padding with '\0' padding in
accordance with [1] and expectations of guest OS

1) ACPI spec, v2.0b
       17.2 AML Grammar Definition
       ...
       //OEM ID of up to 6 characters. If the OEM ID is
       //shorter than 6 characters, it can be terminated
       //with a NULL character.

2)
Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/acpi/aml-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b3b3310df3..65148d5b9d 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
     build_append_int_noprefix(array, 0, 4); /* Length */
     build_append_int_noprefix(array, desc->rev, 1); /* Revision */
     build_append_int_noprefix(array, 0, 1); /* Checksum */
-    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
+    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
     /* OEM Table ID */
-    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
+    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
     build_append_int_noprefix(array, 1, 4); /* OEM Revision */
     g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
     build_append_int_noprefix(array, 1, 4); /* Creator Revision */
-- 
2.31.1

[PATCH 4/4] tests: acpi: update expected blobs

[Igor Mammedov]

Expected changes caused by previous commit:

nvdimm ssdt (q35/pc/virt):
  - *     OEM Table ID     "NVDIMM  "
  + *     OEM Table ID     "NVDIMM"

SLIC test FADT (tests/data/acpi/q35/FACP.slic):
  -[010h 0016   8]                 Oem Table ID : "ME      "
  +[010h 0016   8]                 Oem Table ID : "ME"

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   4 ----
 tests/data/acpi/pc/SSDT.dimmpxm             | Bin 734 -> 734 bytes
 tests/data/acpi/q35/FACP.slic               | Bin 244 -> 244 bytes
 tests/data/acpi/q35/SSDT.dimmpxm            | Bin 734 -> 734 bytes
 tests/data/acpi/virt/SSDT.memhp             | Bin 736 -> 736 bytes
 5 files changed, 4 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 7faa8f53be..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,5 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/SSDT.memhp",
-"tests/data/acpi/pc/SSDT.dimmpxm",
-"tests/data/acpi/q35/SSDT.dimmpxm",
-"tests/data/acpi/q35/FACP.slic",
diff --git a/tests/data/acpi/pc/SSDT.dimmpxm b/tests/data/acpi/pc/SSDT.dimmpxm
index a50a961fa1d9b0dd8ea4096d652c83bcf04db20b..ac55387d57e48adb99eb738a102308688a262fb8 100644
GIT binary patch
delta 33
ocmcb|dXH5iIM^lR9uortW0;e_vq!LkUzm%huP+0`Mu}rg0HzrUKL7v#

delta 33
ocmcb|dXH5iIM^lR9uortqnMMwvq!LkUzm%hudjl_Mu}rg0HV1GKL7v#

diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
index 891fd4b784b7b6b3ea303976db7ecd5b669bc84b..15986e095cf2db7ee92f7ce113c1d46d54018c62 100644
GIT binary patch
delta 32
lcmeyu_=Qoz&CxmF3j+fK^CjmX$6yZyUsoUp2qsG00RW!Z2#x>%

delta 32
kcmeyu_=Qoz&CxmF3j+fKvygL;W3Y#Uud4zWOq93-0G2oijsO4v

diff --git a/tests/data/acpi/q35/SSDT.dimmpxm b/tests/data/acpi/q35/SSDT.dimmpxm
index 617a1c911c7d6753bcedc8ecc52e3027a5259ad6..98e6f0e3f3bb02dd419e36bdd1db9b94c728c406 100644
GIT binary patch
delta 33
ocmcb|dXH5iIM^lR9uortqnnezvq!LkUzm%huP+0`Mu}rg0Ho;&F8}}l

delta 33
ocmcb|dXH5iIM^lR9uortBb$@Ivq!LkUzm%hudjl_Mu}rg0HKKqF8}}l

diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
index e8b850ae2239d8f496b12de672c2a1268e2f269d..375d7b6fc85a484f492a26ccd355c205f2c34473 100644
GIT binary patch
delta 33
ocmaFB`hZm;IM^lR0TTlQqrH>Avq!LkUzm%huP+0`Mu`(l0HqiSFaQ7m

delta 33
ocmaFB`hZm;IM^lR0TTlQ<9{cAXOCb7zc3e1Uta}<jS?rA0JOLYFaQ7m

-- 
2.31.1

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Michael S. Tsirkin]

On Wed, Jan 12, 2022 at 08:03:31AM -0500, Igor Mammedov wrote:
> Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> fields in headers of ACPI tables. While it doesn't have impact on
> default values since QEMU uses 6 and 8 characters long values
> respectively, it broke usecase where IDs are provided on QEMU CLI.
> It shouldn't affect guest (but may cause licensing verification
> issues in guest OS).
> One of the broken usecases is user supplied SLIC table with IDs
> shorter than max possible length, where [2] mangles IDs with extra
> spaces in RSDT and FADT tables whereas guest OS expects those to
> mirror the respective values of the used SLIC table.
> 
> Fix it by replacing whitespace padding with '\0' padding in
> accordance with [1] and expectations of guest OS
> 
> 1) ACPI spec, v2.0b
>        17.2 AML Grammar Definition
>        ...
>        //OEM ID of up to 6 characters. If the OEM ID is
>        //shorter than 6 characters, it can be terminated
>        //with a NULL character.
> 
> 2)
> Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")


and add:

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/707

?

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
> Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-stable@nongnu.org
> ---
>  hw/acpi/aml-build.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b3b3310df3..65148d5b9d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
>      build_append_int_noprefix(array, 0, 4); /* Length */
>      build_append_int_noprefix(array, desc->rev, 1); /* Revision */
>      build_append_int_noprefix(array, 0, 1); /* Checksum */
> -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
>      /* OEM Table ID */
> -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
>      build_append_int_noprefix(array, 1, 4); /* OEM Revision */
>      g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
>      build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> -- 
> 2.31.1

Re: [PATCH 1/4] tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields() test

[Michael S. Tsirkin]

On Wed, Jan 12, 2022 at 08:03:29AM -0500, Igor Mammedov wrote:
> The next commit will revert OEM fields padding with whitespace to
> padding with '\0' as it was before [1]. As result test_oem_fields() will
> fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables.
> 
> Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test
> puts on QEMU CLI and expected values match.
> 
> 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

That's kind of ugly in that we do not test
shorter names then.  How about we pad with \0 instead?
And add a comment explaining why it's done.

> ---
>  tests/qtest/bios-tables-test.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index e6b72d9026..90c9f6a0a2 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -71,9 +71,10 @@
>  
>  #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
>  
> -#define OEM_ID             "TEST"
> -#define OEM_TABLE_ID       "OEM"
> -#define OEM_TEST_ARGS      "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID
> +#define OEM_ID             "TEST  "
> +#define OEM_TABLE_ID       "OEM     "
> +#define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
> +                           OEM_TABLE_ID "'"
>  
>  typedef struct {
>      bool tcg_only;
> @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void)
>  static void test_oem_fields(test_data *data)
>  {
>      int i;
> -    char oem_id[6];
> -    char oem_table_id[8];
>  
> -    strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' ');
> -    strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' ');
>      for (i = 0; i < data->tables->len; ++i) {
>          AcpiSdtTable *sdt;
>  
> @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data)
>              continue;
>          }
>  
> -        g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0);
> -        g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0);
> +        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
> +        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
>      }
>  }
>  
> -- 
> 2.31.1

Re: [PATCH 4/4] tests: acpi: update expected blobs

[Ani Sinha]

On Wed, 12 Jan 2022, Igor Mammedov wrote:

> Expected changes caused by previous commit:
>
> nvdimm ssdt (q35/pc/virt):
>   - *     OEM Table ID     "NVDIMM  "
>   + *     OEM Table ID     "NVDIMM"
>
> SLIC test FADT (tests/data/acpi/q35/FACP.slic):
>   -[010h 0016   8]                 Oem Table ID : "ME      "
>   +[010h 0016   8]                 Oem Table ID : "ME"
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Acked-by: Ani Sinha <ani@anisinha.ca>

> ---
>  tests/qtest/bios-tables-test-allowed-diff.h |   4 ----
>  tests/data/acpi/pc/SSDT.dimmpxm             | Bin 734 -> 734 bytes
>  tests/data/acpi/q35/FACP.slic               | Bin 244 -> 244 bytes
>  tests/data/acpi/q35/SSDT.dimmpxm            | Bin 734 -> 734 bytes
>  tests/data/acpi/virt/SSDT.memhp             | Bin 736 -> 736 bytes
>  5 files changed, 4 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 7faa8f53be..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,5 +1 @@
>  /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/virt/SSDT.memhp",
> -"tests/data/acpi/pc/SSDT.dimmpxm",
> -"tests/data/acpi/q35/SSDT.dimmpxm",
> -"tests/data/acpi/q35/FACP.slic",
> diff --git a/tests/data/acpi/pc/SSDT.dimmpxm b/tests/data/acpi/pc/SSDT.dimmpxm
> index a50a961fa1d9b0dd8ea4096d652c83bcf04db20b..ac55387d57e48adb99eb738a102308688a262fb8 100644
> GIT binary patch
> delta 33
> ocmcb|dXH5iIM^lR9uortW0;e_vq!LkUzm%huP+0`Mu}rg0HzrUKL7v#
>
> delta 33
> ocmcb|dXH5iIM^lR9uortqnMMwvq!LkUzm%hudjl_Mu}rg0HV1GKL7v#
>
> diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
> index 891fd4b784b7b6b3ea303976db7ecd5b669bc84b..15986e095cf2db7ee92f7ce113c1d46d54018c62 100644
> GIT binary patch
> delta 32
> lcmeyu_=Qoz&CxmF3j+fK^CjmX$6yZyUsoUp2qsG00RW!Z2#x>%
>
> delta 32
> kcmeyu_=Qoz&CxmF3j+fKvygL;W3Y#Uud4zWOq93-0G2oijsO4v
>
> diff --git a/tests/data/acpi/q35/SSDT.dimmpxm b/tests/data/acpi/q35/SSDT.dimmpxm
> index 617a1c911c7d6753bcedc8ecc52e3027a5259ad6..98e6f0e3f3bb02dd419e36bdd1db9b94c728c406 100644
> GIT binary patch
> delta 33
> ocmcb|dXH5iIM^lR9uortqnnezvq!LkUzm%huP+0`Mu}rg0Ho;&F8}}l
>
> delta 33
> ocmcb|dXH5iIM^lR9uortBb$@Ivq!LkUzm%hudjl_Mu}rg0HKKqF8}}l
>
> diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
> index e8b850ae2239d8f496b12de672c2a1268e2f269d..375d7b6fc85a484f492a26ccd355c205f2c34473 100644
> GIT binary patch
> delta 33
> ocmaFB`hZm;IM^lR0TTlQqrH>Avq!LkUzm%huP+0`Mu`(l0HqiSFaQ7m
>
> delta 33
> ocmaFB`hZm;IM^lR0TTlQ<9{cAXOCb7zc3e1Uta}<jS?rA0JOLYFaQ7m
>
> --
> 2.31.1
>
>

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Wed, 12 Jan 2022, Michael S. Tsirkin wrote:

> On Wed, Jan 12, 2022 at 08:03:31AM -0500, Igor Mammedov wrote:
> > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > fields in headers of ACPI tables. While it doesn't have impact on
> > default values since QEMU uses 6 and 8 characters long values
> > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > It shouldn't affect guest (but may cause licensing verification
> > issues in guest OS).
> > One of the broken usecases is user supplied SLIC table with IDs
> > shorter than max possible length, where [2] mangles IDs with extra
> > spaces in RSDT and FADT tables whereas guest OS expects those to
> > mirror the respective values of the used SLIC table.
> >
> > Fix it by replacing whitespace padding with '\0' padding in
> > accordance with [1] and expectations of guest OS
> >
> > 1) ACPI spec, v2.0b
> >        17.2 AML Grammar Definition
> >        ...
> >        //OEM ID of up to 6 characters. If the OEM ID is
> >        //shorter than 6 characters, it can be terminated
> >        //with a NULL character.
> >
> > 2)
> > Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
>
>
> and add:
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/707

He did that already with the "Resolves:" line below.
>
> ?
>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
> > Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > ---
> >  hw/acpi/aml-build.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index b3b3310df3..65148d5b9d 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
> >      build_append_int_noprefix(array, 0, 4); /* Length */
> >      build_append_int_noprefix(array, desc->rev, 1); /* Revision */
> >      build_append_int_noprefix(array, 0, 1); /* Checksum */
> > -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> > +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
> >      /* OEM Table ID */
> > -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> > +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
> >      build_append_int_noprefix(array, 1, 4); /* OEM Revision */
> >      g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
> >      build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> > --
> > 2.31.1
>
>

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Wed, 12 Jan 2022, Igor Mammedov wrote:

> Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> fields in headers of ACPI tables. While it doesn't have impact on
> default values since QEMU uses 6 and 8 characters long values
> respectively, it broke usecase where IDs are provided on QEMU CLI.
> It shouldn't affect guest (but may cause licensing verification
> issues in guest OS).
> One of the broken usecases is user supplied SLIC table with IDs
> shorter than max possible length, where [2] mangles IDs with extra
> spaces in RSDT and FADT tables whereas guest OS expects those to
> mirror the respective values of the used SLIC table.
>
> Fix it by replacing whitespace padding with '\0' padding in
> accordance with [1] and expectations of guest OS
>
> 1) ACPI spec, v2.0b
>        17.2 AML Grammar Definition
>        ...
>        //OEM ID of up to 6 characters. If the OEM ID is
>        //shorter than 6 characters, it can be terminated
>        //with a NULL character.
>
> 2)
> Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
> Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-stable@nongnu.org

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/acpi/aml-build.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b3b3310df3..65148d5b9d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
>      build_append_int_noprefix(array, 0, 4); /* Length */
>      build_append_int_noprefix(array, desc->rev, 1); /* Revision */
>      build_append_int_noprefix(array, 0, 1); /* Checksum */
> -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
>      /* OEM Table ID */
> -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
>      build_append_int_noprefix(array, 1, 4); /* OEM Revision */
>      g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
>      build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> --
> 2.31.1
>
>

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Michael S. Tsirkin]

On Wed, Jan 12, 2022 at 08:46:16PM +0530, Ani Sinha wrote:
> 
> 
> On Wed, 12 Jan 2022, Michael S. Tsirkin wrote:
> 
> > On Wed, Jan 12, 2022 at 08:03:31AM -0500, Igor Mammedov wrote:
> > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > fields in headers of ACPI tables. While it doesn't have impact on
> > > default values since QEMU uses 6 and 8 characters long values
> > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > It shouldn't affect guest (but may cause licensing verification
> > > issues in guest OS).
> > > One of the broken usecases is user supplied SLIC table with IDs
> > > shorter than max possible length, where [2] mangles IDs with extra
> > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > mirror the respective values of the used SLIC table.
> > >
> > > Fix it by replacing whitespace padding with '\0' padding in
> > > accordance with [1] and expectations of guest OS
> > >
> > > 1) ACPI spec, v2.0b
> > >        17.2 AML Grammar Definition
> > >        ...
> > >        //OEM ID of up to 6 characters. If the OEM ID is
> > >        //shorter than 6 characters, it can be terminated
> > >        //with a NULL character.
> > >
> > > 2)
> > > Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> >
> >
> > and add:
> >
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/707
> 
> He did that already with the "Resolves:" line below.

oh sorry

> >
> > ?
> >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
> > > Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Cc: qemu-stable@nongnu.org
> > > ---
> > >  hw/acpi/aml-build.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index b3b3310df3..65148d5b9d 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
> > >      build_append_int_noprefix(array, 0, 4); /* Length */
> > >      build_append_int_noprefix(array, desc->rev, 1); /* Revision */
> > >      build_append_int_noprefix(array, 0, 1); /* Checksum */
> > > -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> > > +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
> > >      /* OEM Table ID */
> > > -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> > > +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
> > >      build_append_int_noprefix(array, 1, 4); /* OEM Revision */
> > >      g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
> > >      build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> > > --
> > > 2.31.1
> >
> >

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Dmitry V. Orekhov]

[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]

On 1/12/22 16:03, Igor Mammedov wrote:
> Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> fields in headers of ACPI tables. While it doesn't have impact on
> default values since QEMU uses 6 and 8 characters long values
> respectively, it broke usecase where IDs are provided on QEMU CLI.
> It shouldn't affect guest (but may cause licensing verification
> issues in guest OS).
> One of the broken usecases is user supplied SLIC table with IDs
> shorter than max possible length, where [2] mangles IDs with extra
> spaces in RSDT and FADT tables whereas guest OS expects those to
> mirror the respective values of the used SLIC table.
>
> Fix it by replacing whitespace padding with '\0' padding in
> accordance with [1] and expectations of guest OS
>
> 1) ACPI spec, v2.0b
>         17.2 AML Grammar Definition
>         ...
>         //OEM ID of up to 6 characters. If the OEM ID is
>         //shorter than 6 characters, it can be terminated
>         //with a NULL character.
>
> 2)
> Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/707
> Reported-by: Dmitry V. Orekhov<dima.orekhov@gmail.com>
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> Cc:qemu-stable@nongnu.org
> ---
>   hw/acpi/aml-build.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b3b3310df3..65148d5b9d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
>       build_append_int_noprefix(array, 0, 4); /* Length */
>       build_append_int_noprefix(array, desc->rev, 1); /* Revision */
>       build_append_int_noprefix(array, 0, 1); /* Checksum */
> -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
>       /* OEM Table ID */
> -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
>       build_append_int_noprefix(array, 1, 4); /* OEM Revision */
>       g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
>       build_append_int_noprefix(array, 1, 4); /* Creator Revision */

I can't apply the patch to the qemu-6.1.0 source code on my own.
There is no acpi_table_begin function in the qemu-6.1.0 source code (hw/acpi/aml-buld.c).

[-- Attachment #2: Type: text/html, Size: 3113 bytes --]

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Thu, 13 Jan 2022, Dmitry V. Orekhov wrote:
> I can't apply the patch to the qemu-6.1.0 source code on my own.
> There is no acpi_table_begin function in the qemu-6.1.0 source code
> (hw/acpi/aml-buld.c).
>
Try the following patch :

From 10620c384bf05f0a7561c1afd0ec8ad5af9b7c0f Mon Sep 17 00:00:00 2001
From: Ani Sinha <ani@anisinha.ca>
Date: Thu, 13 Jan 2022 15:48:16 +0530
Subject: [PATCH] acpi: fix OEM ID/OEM Table ID padding for qemu 6.1.1

Replace whitespace padding with '\0' padding in accordance with spec
and expectations of guest OS.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/acpi/aml-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d5103e6..0df053c 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1703,9 +1703,9 @@ build_header(BIOSLinker *linker, GArray *table_data,
     h->length = cpu_to_le32(len);
     h->revision = rev;

-    strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, ' ');
+    strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, '\0');
     strpadcpy((char *)h->oem_table_id, sizeof h->oem_table_id,
-              oem_table_id, ' ');
+              oem_table_id, '\0');

     h->oem_revision = cpu_to_le32(1);
     memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME8, 4);
-- 
2.6.3

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Dmitry V. Orekhov]

On 1/13/22 13:22, Ani Sinha wrote:
>
> On Thu, 13 Jan 2022, Dmitry V. Orekhov wrote:
>> I can't apply the patch to the qemu-6.1.0 source code on my own.
>> There is no acpi_table_begin function in the qemu-6.1.0 source code
>> (hw/acpi/aml-buld.c).
>>
> Try the following patch :
>
>  From 10620c384bf05f0a7561c1afd0ec8ad5af9b7c0f Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@anisinha.ca>
> Date: Thu, 13 Jan 2022 15:48:16 +0530
> Subject: [PATCH] acpi: fix OEM ID/OEM Table ID padding for qemu 6.1.1
>
> Replace whitespace padding with '\0' padding in accordance with spec
> and expectations of guest OS.
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>   hw/acpi/aml-build.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index d5103e6..0df053c 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1703,9 +1703,9 @@ build_header(BIOSLinker *linker, GArray *table_data,
>       h->length = cpu_to_le32(len);
>       h->revision = rev;
>
> -    strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, ' ');
> +    strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, '\0');
>       strpadcpy((char *)h->oem_table_id, sizeof h->oem_table_id,
> -              oem_table_id, ' ');
> +              oem_table_id, '\0');
>
>       h->oem_revision = cpu_to_le32(1);
>       memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME8, 4);

The problem has been solved. Thanks.

Tested-by: Dmitry V. Orekhov dima.orekhov@gmail.com

Re: [PATCH 1/4] tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields() test

[Igor Mammedov]

On Wed, 12 Jan 2022 08:44:19 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 12, 2022 at 08:03:29AM -0500, Igor Mammedov wrote:
> > The next commit will revert OEM fields padding with whitespace to
> > padding with '\0' as it was before [1]. As result test_oem_fields() will
> > fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables.
> > 
> > Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test
> > puts on QEMU CLI and expected values match.
> > 
> > 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> That's kind of ugly in that we do not test
> shorter names then.  How about we pad with \0 instead?


test_acpi_q35_slic() should cover short OEM_TABLE_ID.

also padding in this patch makes test_oem_fields() cleaner
and simplifies 3/4, switching to \0 here would require
merging this patch with the fix itself to avoid breaking
bisection.

If you still prefer to have test_oem_fields() test short
names, I can post following on top that can to it without
breaking bisection:

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 90c9f6a0a2..0fd7cf1f89 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -71,8 +71,8 @@
 
 #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
 
-#define OEM_ID             "TEST  "
-#define OEM_TABLE_ID       "OEM     "
+#define OEM_ID             "TEST"
+#define OEM_TABLE_ID       "OEM"
 #define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
                            OEM_TABLE_ID "'"
 
@@ -1530,8 +1530,8 @@ static void test_oem_fields(test_data *data)
             continue;
         }
 
-        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
-        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
+        g_assert(strncmp((char *)sdt->aml + 10, OEM_ID, 6) == 0);
+        g_assert(strncmp((char *)sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
     }
 }
 


> And add a comment explaining why it's done.
> 
> > ---
> >  tests/qtest/bios-tables-test.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index e6b72d9026..90c9f6a0a2 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -71,9 +71,10 @@
> >  
> >  #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
> >  
> > -#define OEM_ID             "TEST"
> > -#define OEM_TABLE_ID       "OEM"
> > -#define OEM_TEST_ARGS      "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID
> > +#define OEM_ID             "TEST  "
> > +#define OEM_TABLE_ID       "OEM     "
> > +#define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
> > +                           OEM_TABLE_ID "'"
> >  
> >  typedef struct {
> >      bool tcg_only;
> > @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void)
> >  static void test_oem_fields(test_data *data)
> >  {
> >      int i;
> > -    char oem_id[6];
> > -    char oem_table_id[8];
> >  
> > -    strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' ');
> > -    strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' ');
> >      for (i = 0; i < data->tables->len; ++i) {
> >          AcpiSdtTable *sdt;
> >  
> > @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data)
> >              continue;
> >          }
> >  
> > -        g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0);
> > -        g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0);
> > +        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
> > +        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
> >      }
> >  }
> >  
> > -- 
> > 2.31.1  
> 

Re: [PATCH 1/4] tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields() test

[Michael S. Tsirkin]

On Fri, Jan 14, 2022 at 12:48:20PM +0100, Igor Mammedov wrote:
> On Wed, 12 Jan 2022 08:44:19 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 12, 2022 at 08:03:29AM -0500, Igor Mammedov wrote:
> > > The next commit will revert OEM fields padding with whitespace to
> > > padding with '\0' as it was before [1]. As result test_oem_fields() will
> > > fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables.
> > > 
> > > Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test
> > > puts on QEMU CLI and expected values match.
> > > 
> > > 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > That's kind of ugly in that we do not test
> > shorter names then.  How about we pad with \0 instead?
> 
> 
> test_acpi_q35_slic() should cover short OEM_TABLE_ID.
> 
> also padding in this patch makes test_oem_fields() cleaner
> and simplifies 3/4, switching to \0 here would require
> merging this patch with the fix itself to avoid breaking
> bisection.
> 
> If you still prefer to have test_oem_fields() test short
> names, I can post following on top that can to it without
> breaking bisection:
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 90c9f6a0a2..0fd7cf1f89 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -71,8 +71,8 @@
>  
>  #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
>  
> -#define OEM_ID             "TEST  "
> -#define OEM_TABLE_ID       "OEM     "
> +#define OEM_ID             "TEST"
> +#define OEM_TABLE_ID       "OEM"
>  #define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
>                             OEM_TABLE_ID "'"

Don't we want to revert ARGS change too then?


> @@ -1530,8 +1530,8 @@ static void test_oem_fields(test_data *data)
>              continue;
>          }
>  
> -        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
> -        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
> +        g_assert(strncmp((char *)sdt->aml + 10, OEM_ID, 6) == 0);
> +        g_assert(strncmp((char *)sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
>      }
>  }
>  

Looks much cleaner to me. OK as a patch on top.


> 
> > And add a comment explaining why it's done.
> > 
> > > ---
> > >  tests/qtest/bios-tables-test.c | 15 ++++++---------
> > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > > index e6b72d9026..90c9f6a0a2 100644
> > > --- a/tests/qtest/bios-tables-test.c
> > > +++ b/tests/qtest/bios-tables-test.c
> > > @@ -71,9 +71,10 @@
> > >  
> > >  #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
> > >  
> > > -#define OEM_ID             "TEST"
> > > -#define OEM_TABLE_ID       "OEM"
> > > -#define OEM_TEST_ARGS      "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID
> > > +#define OEM_ID             "TEST  "
> > > +#define OEM_TABLE_ID       "OEM     "
> > > +#define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
> > > +                           OEM_TABLE_ID "'"
> > >  
> > >  typedef struct {
> > >      bool tcg_only;
> > > @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void)
> > >  static void test_oem_fields(test_data *data)
> > >  {
> > >      int i;
> > > -    char oem_id[6];
> > > -    char oem_table_id[8];
> > >  
> > > -    strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' ');
> > > -    strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' ');
> > >      for (i = 0; i < data->tables->len; ++i) {
> > >          AcpiSdtTable *sdt;
> > >  
> > > @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data)
> > >              continue;
> > >          }
> > >  
> > > -        g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0);
> > > -        g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0);
> > > +        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
> > > +        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
> > >      }
> > >  }
> > >  
> > > -- 
> > > 2.31.1  
> > 

[PATCH 5/4] tests: acpi: test short OEM_ID/OEM_TABLE_ID values in test_oem_fields()

[Igor Mammedov]

Previous patch [1] added explicit whitespace padding to OEM_ID/OEM_TABLE_ID
values used in test_oem_fields() testcase to avoid false positive and
bisection issues when QEMU is switched to \0' padding. As result
testcase ceased to test values that were shorter than max possible
length values.

Update testcase to make sure that it's testing shorter IDs like it
used to before [2].

1) "tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for  test_oem_fields() test"
2) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 90c9f6a0a2..ad536fd7b1 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -71,10 +71,10 @@
 
 #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
 
-#define OEM_ID             "TEST  "
-#define OEM_TABLE_ID       "OEM     "
-#define OEM_TEST_ARGS      "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \
-                           OEM_TABLE_ID "'"
+#define OEM_ID             "TEST"
+#define OEM_TABLE_ID       "OEM"
+#define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
+                           OEM_TABLE_ID
 
 typedef struct {
     bool tcg_only;
@@ -1530,8 +1530,8 @@ static void test_oem_fields(test_data *data)
             continue;
         }
 
-        g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0);
-        g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
+        g_assert(strncmp((char *)sdt->aml + 10, OEM_ID, 6) == 0);
+        g_assert(strncmp((char *)sdt->aml + 16, OEM_TABLE_ID, 8) == 0);
     }
 }
 
-- 
2.31.1

Re: [PATCH 5/4] tests: acpi: test short OEM_ID/OEM_TABLE_ID values in test_oem_fields()

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On Fri, Jan 14, 2022 at 7:57 PM Igor Mammedov <imammedo@redhat.com> wrote:

> Previous patch [1] added explicit whitespace padding to OEM_ID/OEM_TABLE_ID
> values used in test_oem_fields() testcase to avoid false positive and
> bisection issues when QEMU is switched to \0' padding. As result
> testcase ceased to test values that were shorter than max possible
> length values.
>
> Update testcase to make sure that it's testing shorter IDs like it
> used to before [2].
>
> 1) "tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for  test_oem_fields()
> test"
> 2) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>


Reviewed-by: Ani Sinha <ani@anisinha.ca>



>

[-- Attachment #2: Type: text/html, Size: 1707 bytes --]

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> fields in headers of ACPI tables. While it doesn't have impact on
> default values since QEMU uses 6 and 8 characters long values
> respectively, it broke usecase where IDs are provided on QEMU CLI.
> It shouldn't affect guest (but may cause licensing verification
> issues in guest OS).
> One of the broken usecases is user supplied SLIC table with IDs
> shorter than max possible length, where [2] mangles IDs with extra
> spaces in RSDT and FADT tables whereas guest OS expects those to
> mirror the respective values of the used SLIC table.
>
> Fix it by replacing whitespace padding with '\0' padding in
> accordance with [1] and expectations of guest OS
>
> 1) ACPI spec, v2.0b
>        17.2 AML Grammar Definition
>        ...
>        //OEM ID of up to 6 characters. If the OEM ID is
>        //shorter than 6 characters, it can be terminated
>        //with a NULL character.

On the other hand, from
https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
,

"For example, the OEM ID and OEM Table ID in the common ACPI table
header (shown above) are fixed at six and eight characters,
respectively. They are not necessarily null terminated"

I also checked version 5 and the verbiage is the same. I think not
terminating with a null is not incorrect.

>
> 2)
> Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
> Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-stable@nongnu.org
> ---
>  hw/acpi/aml-build.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b3b3310df3..65148d5b9d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
>      build_append_int_noprefix(array, 0, 4); /* Length */
>      build_append_int_noprefix(array, desc->rev, 1); /* Revision */
>      build_append_int_noprefix(array, 0, 1); /* Checksum */
> -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
>      /* OEM Table ID */
> -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
>      build_append_int_noprefix(array, 1, 4); /* OEM Revision */
>      g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
>      build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> --
> 2.31.1
>

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Igor Mammedov]

On Mon, 31 Jan 2022 11:47:00 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > fields in headers of ACPI tables. While it doesn't have impact on
> > default values since QEMU uses 6 and 8 characters long values
> > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > It shouldn't affect guest (but may cause licensing verification
> > issues in guest OS).
> > One of the broken usecases is user supplied SLIC table with IDs
> > shorter than max possible length, where [2] mangles IDs with extra
> > spaces in RSDT and FADT tables whereas guest OS expects those to
> > mirror the respective values of the used SLIC table.
> >
> > Fix it by replacing whitespace padding with '\0' padding in
> > accordance with [1] and expectations of guest OS
> >
> > 1) ACPI spec, v2.0b
> >        17.2 AML Grammar Definition
> >        ...
> >        //OEM ID of up to 6 characters. If the OEM ID is
> >        //shorter than 6 characters, it can be terminated
> >        //with a NULL character.  
> 
> On the other hand, from
> https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> ,
> 
> "For example, the OEM ID and OEM Table ID in the common ACPI table
> header (shown above) are fixed at six and eight characters,
> respectively. They are not necessarily null terminated"
> 
> I also checked version 5 and the verbiage is the same. I think not
> terminating with a null is not incorrect.

I have a trouble with too much 'not' within the sentence.
So what's the point of this comment and how it's related to
this patch?



> > 2)
> > Fixes: 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/707
> > Reported-by: Dmitry V. Orekhov <dima.orekhov@gmail.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > ---
> >  hw/acpi/aml-build.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index b3b3310df3..65148d5b9d 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1724,9 +1724,9 @@ void acpi_table_begin(AcpiTable *desc, GArray *array)
> >      build_append_int_noprefix(array, 0, 4); /* Length */
> >      build_append_int_noprefix(array, desc->rev, 1); /* Revision */
> >      build_append_int_noprefix(array, 0, 1); /* Checksum */
> > -    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> > +    build_append_padded_str(array, desc->oem_id, 6, '\0'); /* OEMID */
> >      /* OEM Table ID */
> > -    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> > +    build_append_padded_str(array, desc->oem_table_id, 8, '\0');
> >      build_append_int_noprefix(array, 1, 4); /* OEM Revision */
> >      g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
> >      build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> > --
> > 2.31.1
> >  
> 

Re: [PATCH 0/4] acpi: fix short OEM [Table] ID padding

[Igor Mammedov]

On Wed, 12 Jan 2022 08:03:28 -0500
Igor Mammedov <imammedo@redhat.com> wrote:

> Since 6.0 the commit:
>   602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> regressed values of OEM [Table] ID fields in ACPI tables
> by padding them with whitespace is a value is shorter then
> max possible. That depending on vendor broke OEM [Table] ID patching
> with SLIC table values and as result licensing of Windows guests.
> 
> First reported here https://gitlab.com/qemu-project/qemu/-/issues/707

ping,
Michael can you pick it up so that downstreams could
backport the fix?

> 
> CC: Marian Postevca <posteuca@mutex.one>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Ani Sinha <ani@anisinha.ca>
> 
> Igor Mammedov (4):
>   tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields()
>     test
>   tests: acpi: whitelist nvdimm's SSDT and FACP.slic expected blobs
>   acpi: fix OEM ID/OEM Table ID padding
>   tests: acpi: update expected blobs
> 
>  hw/acpi/aml-build.c              |   4 ++--
>  tests/data/acpi/pc/SSDT.dimmpxm  | Bin 734 -> 734 bytes
>  tests/data/acpi/q35/FACP.slic    | Bin 244 -> 244 bytes
>  tests/data/acpi/q35/SSDT.dimmpxm | Bin 734 -> 734 bytes
>  tests/data/acpi/virt/SSDT.memhp  | Bin 736 -> 736 bytes
>  tests/qtest/bios-tables-test.c   |  15 ++++++---------
>  6 files changed, 8 insertions(+), 11 deletions(-)
> 

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Mon, 31 Jan 2022, Igor Mammedov wrote:

> On Mon, 31 Jan 2022 11:47:00 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > fields in headers of ACPI tables. While it doesn't have impact on
> > > default values since QEMU uses 6 and 8 characters long values
> > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > It shouldn't affect guest (but may cause licensing verification
> > > issues in guest OS).
> > > One of the broken usecases is user supplied SLIC table with IDs
> > > shorter than max possible length, where [2] mangles IDs with extra
> > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > mirror the respective values of the used SLIC table.
> > >
> > > Fix it by replacing whitespace padding with '\0' padding in
> > > accordance with [1] and expectations of guest OS
> > >
> > > 1) ACPI spec, v2.0b
> > >        17.2 AML Grammar Definition
> > >        ...
> > >        //OEM ID of up to 6 characters. If the OEM ID is
> > >        //shorter than 6 characters, it can be terminated
> > >        //with a NULL character.
> >
> > On the other hand, from
> > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > ,
> >
> > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > header (shown above) are fixed at six and eight characters,
> > respectively. They are not necessarily null terminated"
> >
> > I also checked version 5 and the verbiage is the same. I think not
> > terminating with a null is not incorrect.
>
> I have a trouble with too much 'not' within the sentence.

:-)

> So what's the point of this comment and how it's related to
> this patch?

My understanding of the spec is that null termination of both those IDs is
not mandatory. Guests may get confused or expect the strings to be null
termimated but they should really be open to expecting non-null terminated
strings as well. What is important is that the number of chars of those
two strings are fixed and well defined in the spec and qemu
implementation.

In any case, I think we can leave the patch as is for now and see if the
change causes trouble with other guests.

Re: [PATCH 0/4] acpi: fix short OEM [Table] ID padding

[Michael S. Tsirkin]

On Mon, Jan 31, 2022 at 02:21:56PM +0100, Igor Mammedov wrote:
> On Wed, 12 Jan 2022 08:03:28 -0500
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > Since 6.0 the commit:
> >   602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> > regressed values of OEM [Table] ID fields in ACPI tables
> > by padding them with whitespace is a value is shorter then
> > max possible. That depending on vendor broke OEM [Table] ID patching
> > with SLIC table values and as result licensing of Windows guests.
> > 
> > First reported here https://gitlab.com/qemu-project/qemu/-/issues/707
> 
> ping,
> Michael can you pick it up so that downstreams could
> backport the fix?

I see the differences with Ani have been resolved, will be in
the next pull.

> > 
> > CC: Marian Postevca <posteuca@mutex.one>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Ani Sinha <ani@anisinha.ca>
> > 
> > Igor Mammedov (4):
> >   tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields()
> >     test
> >   tests: acpi: whitelist nvdimm's SSDT and FACP.slic expected blobs
> >   acpi: fix OEM ID/OEM Table ID padding
> >   tests: acpi: update expected blobs
> > 
> >  hw/acpi/aml-build.c              |   4 ++--
> >  tests/data/acpi/pc/SSDT.dimmpxm  | Bin 734 -> 734 bytes
> >  tests/data/acpi/q35/FACP.slic    | Bin 244 -> 244 bytes
> >  tests/data/acpi/q35/SSDT.dimmpxm | Bin 734 -> 734 bytes
> >  tests/data/acpi/virt/SSDT.memhp  | Bin 736 -> 736 bytes
> >  tests/qtest/bios-tables-test.c   |  15 ++++++---------
> >  6 files changed, 8 insertions(+), 11 deletions(-)
> > 

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Igor Mammedov]

On Mon, 31 Jan 2022 18:58:57 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Mon, 31 Jan 2022, Igor Mammedov wrote:
> 
> > On Mon, 31 Jan 2022 11:47:00 +0530
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:  
> > > >
> > > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > > fields in headers of ACPI tables. While it doesn't have impact on
> > > > default values since QEMU uses 6 and 8 characters long values
> > > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > > It shouldn't affect guest (but may cause licensing verification
> > > > issues in guest OS).
> > > > One of the broken usecases is user supplied SLIC table with IDs
> > > > shorter than max possible length, where [2] mangles IDs with extra
> > > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > > mirror the respective values of the used SLIC table.
> > > >
> > > > Fix it by replacing whitespace padding with '\0' padding in
> > > > accordance with [1] and expectations of guest OS
> > > >
> > > > 1) ACPI spec, v2.0b
> > > >        17.2 AML Grammar Definition
> > > >        ...
> > > >        //OEM ID of up to 6 characters. If the OEM ID is
> > > >        //shorter than 6 characters, it can be terminated
> > > >        //with a NULL character.  
> > >
> > > On the other hand, from
> > > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > > ,
> > >
> > > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > > header (shown above) are fixed at six and eight characters,
> > > respectively. They are not necessarily null terminated"
> > >
> > > I also checked version 5 and the verbiage is the same. I think not
> > > terminating with a null is not incorrect.  
> >
> > I have a trouble with too much 'not' within the sentence.  
> 
> :-)
> 
> > So what's the point of this comment and how it's related to
> > this patch?  
> 
> My understanding of the spec is that null termination of both those IDs is
> not mandatory. Guests may get confused or expect the strings to be null
> termimated but they should really be open to expecting non-null terminated
> strings as well. What is important is that the number of chars of those
> two strings are fixed and well defined in the spec and qemu
> implementation.
>
> In any case, I think we can leave the patch as is for now and see if the
> change causes trouble with other guests.


these fields have a fixed length so one doesn't need terminating NULL
in case the full length of the field is utilized, otherwise in case of
where the value is shorter than max length it has to be null terminated
to express a shorter value. That way QEMU worked for years until
602b458201 introduced regression.

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Mon, 31 Jan 2022, Igor Mammedov wrote:

> On Mon, 31 Jan 2022 18:58:57 +0530 (IST)
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> >
> > > On Mon, 31 Jan 2022 11:47:00 +0530
> > > Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > >
> > > > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > > > fields in headers of ACPI tables. While it doesn't have impact on
> > > > > default values since QEMU uses 6 and 8 characters long values
> > > > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > > > It shouldn't affect guest (but may cause licensing verification
> > > > > issues in guest OS).
> > > > > One of the broken usecases is user supplied SLIC table with IDs
> > > > > shorter than max possible length, where [2] mangles IDs with extra
> > > > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > > > mirror the respective values of the used SLIC table.
> > > > >
> > > > > Fix it by replacing whitespace padding with '\0' padding in
> > > > > accordance with [1] and expectations of guest OS
> > > > >
> > > > > 1) ACPI spec, v2.0b
> > > > >        17.2 AML Grammar Definition
> > > > >        ...
> > > > >        //OEM ID of up to 6 characters. If the OEM ID is
> > > > >        //shorter than 6 characters, it can be terminated
> > > > >        //with a NULL character.
> > > >
> > > > On the other hand, from
> > > > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > > > ,
> > > >
> > > > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > > > header (shown above) are fixed at six and eight characters,
> > > > respectively. They are not necessarily null terminated"
> > > >
> > > > I also checked version 5 and the verbiage is the same. I think not
> > > > terminating with a null is not incorrect.
> > >
> > > I have a trouble with too much 'not' within the sentence.
> >
> > :-)
> >
> > > So what's the point of this comment and how it's related to
> > > this patch?
> >
> > My understanding of the spec is that null termination of both those IDs is
> > not mandatory. Guests may get confused or expect the strings to be null
> > termimated but they should really be open to expecting non-null terminated
> > strings as well. What is important is that the number of chars of those
> > two strings are fixed and well defined in the spec and qemu
> > implementation.
> >
> > In any case, I think we can leave the patch as is for now and see if the
> > change causes trouble with other guests.
>
>
> these fields have a fixed length so one doesn't need terminating NULL
> in case the full length of the field is utilized, otherwise in case of
> where the value is shorter than max length it has to be null terminated
> to express a shorter value. That way QEMU worked for years until
> 602b458201 introduced regression.
>

My comment was based on what I interpreted from reading the latest
version of the specs. I guess the spec does not explicitly say what the
padding
bytes would be in case the length of the IDs are less the max length. I
interpreted the wording to mean that whether or not the
length of the string is shorter, one should not expect it to terminate with null.

It would be nice if a future version of the spec made is explicit and
clearer.

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Igor Mammedov]

On Mon, 31 Jan 2022 19:51:24 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Mon, 31 Jan 2022, Igor Mammedov wrote:
> 
> > On Mon, 31 Jan 2022 18:58:57 +0530 (IST)
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> > >  
> > > > On Mon, 31 Jan 2022 11:47:00 +0530
> > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > >  
> > > > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:  
> > > > > >
> > > > > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > > > > fields in headers of ACPI tables. While it doesn't have impact on
> > > > > > default values since QEMU uses 6 and 8 characters long values
> > > > > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > > > > It shouldn't affect guest (but may cause licensing verification
> > > > > > issues in guest OS).
> > > > > > One of the broken usecases is user supplied SLIC table with IDs
> > > > > > shorter than max possible length, where [2] mangles IDs with extra
> > > > > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > > > > mirror the respective values of the used SLIC table.
> > > > > >
> > > > > > Fix it by replacing whitespace padding with '\0' padding in
> > > > > > accordance with [1] and expectations of guest OS
> > > > > >
> > > > > > 1) ACPI spec, v2.0b
> > > > > >        17.2 AML Grammar Definition
> > > > > >        ...
> > > > > >        //OEM ID of up to 6 characters. If the OEM ID is
> > > > > >        //shorter than 6 characters, it can be terminated
> > > > > >        //with a NULL character.  
> > > > >
> > > > > On the other hand, from
> > > > > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > > > > ,
> > > > >
> > > > > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > > > > header (shown above) are fixed at six and eight characters,
> > > > > respectively. They are not necessarily null terminated"
> > > > >
> > > > > I also checked version 5 and the verbiage is the same. I think not
> > > > > terminating with a null is not incorrect.  
> > > >
> > > > I have a trouble with too much 'not' within the sentence.  
> > >
> > > :-)
> > >  
> > > > So what's the point of this comment and how it's related to
> > > > this patch?  
> > >
> > > My understanding of the spec is that null termination of both those IDs is
> > > not mandatory. Guests may get confused or expect the strings to be null
> > > termimated but they should really be open to expecting non-null terminated
> > > strings as well. What is important is that the number of chars of those
> > > two strings are fixed and well defined in the spec and qemu
> > > implementation.
> > >
> > > In any case, I think we can leave the patch as is for now and see if the
> > > change causes trouble with other guests.  
> >
> >
> > these fields have a fixed length so one doesn't need terminating NULL
> > in case the full length of the field is utilized, otherwise in case of
> > where the value is shorter than max length it has to be null terminated
> > to express a shorter value. That way QEMU worked for years until
> > 602b458201 introduced regression.
> >  
> 
> My comment was based on what I interpreted from reading the latest
> version of the specs. I guess the spec does not explicitly say what the
> padding
> bytes would be in case the length of the IDs are less the max length. I
> interpreted the wording to mean that whether or not the
> length of the string is shorter, one should not expect it to terminate with null.

that's what AML grmamar quoted in commit message clarifies
for specific field(s), as opposed to your generic string
type description

> It would be nice if a future version of the spec made is explicit and
> clearer.


PS:
you were asking the other day if there is any bugs left in ACPI,
(the answer is that I'm not aware of any).
But there are issues with SMBIOS tables that need to be fixed
(it's corner cases with large VM configurations), are you
interested in trying to fix it?

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Ani Sinha]

On Tue, 1 Feb 2022, Igor Mammedov wrote:

> On Mon, 31 Jan 2022 19:51:24 +0530 (IST)
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> >
> > > On Mon, 31 Jan 2022 18:58:57 +0530 (IST)
> > > Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > > > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> > > >
> > > > > On Mon, 31 Jan 2022 11:47:00 +0530
> > > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > > >
> > > > > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > > >
> > > > > > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > > > > > fields in headers of ACPI tables. While it doesn't have impact on
> > > > > > > default values since QEMU uses 6 and 8 characters long values
> > > > > > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > > > > > It shouldn't affect guest (but may cause licensing verification
> > > > > > > issues in guest OS).
> > > > > > > One of the broken usecases is user supplied SLIC table with IDs
> > > > > > > shorter than max possible length, where [2] mangles IDs with extra
> > > > > > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > > > > > mirror the respective values of the used SLIC table.
> > > > > > >
> > > > > > > Fix it by replacing whitespace padding with '\0' padding in
> > > > > > > accordance with [1] and expectations of guest OS
> > > > > > >
> > > > > > > 1) ACPI spec, v2.0b
> > > > > > >        17.2 AML Grammar Definition
> > > > > > >        ...
> > > > > > >        //OEM ID of up to 6 characters. If the OEM ID is
> > > > > > >        //shorter than 6 characters, it can be terminated
> > > > > > >        //with a NULL character.
> > > > > >
> > > > > > On the other hand, from
> > > > > > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > > > > > ,
> > > > > >
> > > > > > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > > > > > header (shown above) are fixed at six and eight characters,
> > > > > > respectively. They are not necessarily null terminated"
> > > > > >
> > > > > > I also checked version 5 and the verbiage is the same. I think not
> > > > > > terminating with a null is not incorrect.
> > > > >
> > > > > I have a trouble with too much 'not' within the sentence.
> > > >
> > > > :-)
> > > >
> > > > > So what's the point of this comment and how it's related to
> > > > > this patch?
> > > >
> > > > My understanding of the spec is that null termination of both those IDs is
> > > > not mandatory. Guests may get confused or expect the strings to be null
> > > > termimated but they should really be open to expecting non-null terminated
> > > > strings as well. What is important is that the number of chars of those
> > > > two strings are fixed and well defined in the spec and qemu
> > > > implementation.
> > > >
> > > > In any case, I think we can leave the patch as is for now and see if the
> > > > change causes trouble with other guests.
> > >
> > >
> > > these fields have a fixed length so one doesn't need terminating NULL
> > > in case the full length of the field is utilized, otherwise in case of
> > > where the value is shorter than max length it has to be null terminated
> > > to express a shorter value. That way QEMU worked for years until
> > > 602b458201 introduced regression.
> > >
> >
> > My comment was based on what I interpreted from reading the latest
> > version of the specs. I guess the spec does not explicitly say what the
> > padding
> > bytes would be in case the length of the IDs are less the max length. I
> > interpreted the wording to mean that whether or not the
> > length of the string is shorter, one should not expect it to terminate with null.
>
> that's what AML grmamar quoted in commit message clarifies
> for specific field(s), as opposed to your generic string
> type description

Ah yes, my bad. In
https://uefi.org/specs/ACPI/6.4/20_AML_Specification/AML_Specification.html ,
section 20.2.1 has this also :

ByteData(6) // OEM ID of up to 6 characters. If the OEM ID is shorter than
6 characters,
it can be terminated with a NULL character.

etc. Somehow I missed it.
>
> PS:
> you were asking the other day if there is any bugs left in ACPI,
> (the answer is that I'm not aware of any).

Yes spoke to Gerd offline. On native side also he is unaware of any issues
post 6.2.

> But there are issues with SMBIOS tables that need to be fixed
> (it's corner cases with large VM configurations), are you
> interested in trying to fix it?

Yes sure. I will try my best.

Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Igor Mammedov]

On Tue, 1 Feb 2022 13:25:20 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Tue, 1 Feb 2022, Igor Mammedov wrote:
> 
> > On Mon, 31 Jan 2022 19:51:24 +0530 (IST)
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> > >  
> > > > On Mon, 31 Jan 2022 18:58:57 +0530 (IST)
> > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > >  
> > > > > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> > > > >  
> > > > > > On Mon, 31 Jan 2022 11:47:00 +0530
> > > > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > > > >  
> > > > > > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> wrote:  
> > > > > > > >
> > > > > > > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > > > > > > fields in headers of ACPI tables. While it doesn't have impact on
> > > > > > > > default values since QEMU uses 6 and 8 characters long values
> > > > > > > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > > > > > > It shouldn't affect guest (but may cause licensing verification
> > > > > > > > issues in guest OS).
> > > > > > > > One of the broken usecases is user supplied SLIC table with IDs
> > > > > > > > shorter than max possible length, where [2] mangles IDs with extra
> > > > > > > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > > > > > > mirror the respective values of the used SLIC table.
> > > > > > > >
> > > > > > > > Fix it by replacing whitespace padding with '\0' padding in
> > > > > > > > accordance with [1] and expectations of guest OS
> > > > > > > >
> > > > > > > > 1) ACPI spec, v2.0b
> > > > > > > >        17.2 AML Grammar Definition
> > > > > > > >        ...
> > > > > > > >        //OEM ID of up to 6 characters. If the OEM ID is
> > > > > > > >        //shorter than 6 characters, it can be terminated
> > > > > > > >        //with a NULL character.  
> > > > > > >
> > > > > > > On the other hand, from
> > > > > > > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > > > > > > ,
> > > > > > >
> > > > > > > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > > > > > > header (shown above) are fixed at six and eight characters,
> > > > > > > respectively. They are not necessarily null terminated"
> > > > > > >
> > > > > > > I also checked version 5 and the verbiage is the same. I think not
> > > > > > > terminating with a null is not incorrect.  
> > > > > >
> > > > > > I have a trouble with too much 'not' within the sentence.  
> > > > >
> > > > > :-)
> > > > >  
> > > > > > So what's the point of this comment and how it's related to
> > > > > > this patch?  
> > > > >
> > > > > My understanding of the spec is that null termination of both those IDs is
> > > > > not mandatory. Guests may get confused or expect the strings to be null
> > > > > termimated but they should really be open to expecting non-null terminated
> > > > > strings as well. What is important is that the number of chars of those
> > > > > two strings are fixed and well defined in the spec and qemu
> > > > > implementation.
> > > > >
> > > > > In any case, I think we can leave the patch as is for now and see if the
> > > > > change causes trouble with other guests.  
> > > >
> > > >
> > > > these fields have a fixed length so one doesn't need terminating NULL
> > > > in case the full length of the field is utilized, otherwise in case of
> > > > where the value is shorter than max length it has to be null terminated
> > > > to express a shorter value. That way QEMU worked for years until
> > > > 602b458201 introduced regression.
> > > >  
> > >
> > > My comment was based on what I interpreted from reading the latest
> > > version of the specs. I guess the spec does not explicitly say what the
> > > padding
> > > bytes would be in case the length of the IDs are less the max length. I
> > > interpreted the wording to mean that whether or not the
> > > length of the string is shorter, one should not expect it to terminate with null.  
> >
> > that's what AML grmamar quoted in commit message clarifies
> > for specific field(s), as opposed to your generic string
> > type description  
> 
> Ah yes, my bad. In
> https://uefi.org/specs/ACPI/6.4/20_AML_Specification/AML_Specification.html ,
> section 20.2.1 has this also :
> 
> ByteData(6) // OEM ID of up to 6 characters. If the OEM ID is shorter than
> 6 characters,
> it can be terminated with a NULL character.
> 
> etc. Somehow I missed it.
> >
> > PS:
> > you were asking the other day if there is any bugs left in ACPI,
> > (the answer is that I'm not aware of any).  
> 
> Yes spoke to Gerd offline. On native side also he is unaware of any issues
> post 6.2.
>
> > But there are issues with SMBIOS tables that need to be fixed
> > (it's corner cases with large VM configurations), are you
> > interested in trying to fix it?  
> 
> Yes sure. I will try my best.
> 

Thanks,
here is SMBIOS corruption bug from backlog, created by
Eduardo when he was investigating the issue
https://bugzilla.redhat.com/show_bug.cgi?id=2023977