* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-12 13:03 ` [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding Igor Mammedov
@ 2022-01-12 13:39 ` Michael S. Tsirkin
2022-01-12 15:16 ` Ani Sinha
2022-01-12 15:19 ` Ani Sinha
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-01-12 13:39 UTC (permalink / raw)
To: Igor Mammedov
Cc: Ani Sinha, qemu-stable, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-12 13:39 ` Michael S. Tsirkin
@ 2022-01-12 15:16 ` Ani Sinha
2022-01-12 15:28 ` Michael S. Tsirkin
0 siblings, 1 reply; 30+ messages in thread
From: Ani Sinha @ 2022-01-12 15:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, qemu-stable, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-12 15:16 ` Ani Sinha
@ 2022-01-12 15:28 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-01-12 15:28 UTC (permalink / raw)
To: Ani Sinha
Cc: Igor Mammedov, qemu-stable, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
> >
> >
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-12 13:03 ` [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding Igor Mammedov
2022-01-12 13:39 ` Michael S. Tsirkin
@ 2022-01-12 15:19 ` Ani Sinha
2022-01-13 9:53 ` Dmitry V. Orekhov
2022-01-31 6:17 ` Ani Sinha
3 siblings, 0 replies; 30+ messages in thread
From: Ani Sinha @ 2022-01-12 15:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-stable, Michael S . Tsirkin, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-12 13:03 ` [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding Igor Mammedov
2022-01-12 13:39 ` Michael S. Tsirkin
2022-01-12 15:19 ` Ani Sinha
@ 2022-01-13 9:53 ` Dmitry V. Orekhov
2022-01-13 10:22 ` Ani Sinha
2022-01-31 6:17 ` Ani Sinha
3 siblings, 1 reply; 30+ messages in thread
From: Dmitry V. Orekhov @ 2022-01-13 9:53 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
Cc: Ani Sinha, qemu-stable, Marian Postevca, Michael S . Tsirkin
[-- 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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-13 9:53 ` Dmitry V. Orekhov
@ 2022-01-13 10:22 ` Ani Sinha
2022-01-13 13:19 ` Dmitry V. Orekhov
0 siblings, 1 reply; 30+ messages in thread
From: Ani Sinha @ 2022-01-13 10:22 UTC (permalink / raw)
To: Dmitry V. Orekhov
Cc: Igor Mammedov, qemu-stable, Michael S . Tsirkin, qemu-devel,
Marian Postevca
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-13 10:22 ` Ani Sinha
@ 2022-01-13 13:19 ` Dmitry V. Orekhov
0 siblings, 0 replies; 30+ messages in thread
From: Dmitry V. Orekhov @ 2022-01-13 13:19 UTC (permalink / raw)
To: Ani Sinha
Cc: Igor Mammedov, qemu-stable, Michael S . Tsirkin, qemu-devel,
Marian Postevca
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-12 13:03 ` [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding Igor Mammedov
` (2 preceding siblings ...)
2022-01-13 9:53 ` Dmitry V. Orekhov
@ 2022-01-31 6:17 ` Ani Sinha
2022-01-31 13:20 ` Igor Mammedov
3 siblings, 1 reply; 30+ messages in thread
From: Ani Sinha @ 2022-01-31 6:17 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-stable, Michael S . Tsirkin, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-31 6:17 ` Ani Sinha
@ 2022-01-31 13:20 ` Igor Mammedov
2022-01-31 13:28 ` Ani Sinha
0 siblings, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2022-01-31 13:20 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-stable, Michael S . Tsirkin, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
> >
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-31 13:20 ` Igor Mammedov
@ 2022-01-31 13:28 ` Ani Sinha
2022-01-31 14:10 ` Igor Mammedov
0 siblings, 1 reply; 30+ messages in thread
From: Ani Sinha @ 2022-01-31 13:28 UTC (permalink / raw)
To: Igor Mammedov
Cc: Michael S . Tsirkin, qemu-stable, qemu-devel, Marian Postevca,
Dmitry V . Orekhov, 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-31 13:28 ` Ani Sinha
@ 2022-01-31 14:10 ` Igor Mammedov
2022-01-31 14:21 ` Ani Sinha
0 siblings, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2022-01-31 14:10 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-stable, Michael S . Tsirkin, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-31 14:10 ` Igor Mammedov
@ 2022-01-31 14:21 ` Ani Sinha
2022-02-01 7:39 ` Igor Mammedov
0 siblings, 1 reply; 30+ messages in thread
From: Ani Sinha @ 2022-01-31 14:21 UTC (permalink / raw)
To: Igor Mammedov
Cc: Michael S . Tsirkin, qemu-stable, qemu-devel, Marian Postevca,
Dmitry V . Orekhov, 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-01-31 14:21 ` Ani Sinha
@ 2022-02-01 7:39 ` Igor Mammedov
2022-02-01 7:55 ` Ani Sinha
0 siblings, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2022-02-01 7:39 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-stable, Michael S . Tsirkin, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-02-01 7:39 ` Igor Mammedov
@ 2022-02-01 7:55 ` Ani Sinha
2022-02-01 9:14 ` Igor Mammedov
0 siblings, 1 reply; 30+ messages in thread
From: Ani Sinha @ 2022-02-01 7:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: Michael S . Tsirkin, qemu-stable, qemu-devel, Marian Postevca,
Dmitry V . Orekhov, 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding
2022-02-01 7:55 ` Ani Sinha
@ 2022-02-01 9:14 ` Igor Mammedov
0 siblings, 0 replies; 30+ messages in thread
From: Igor Mammedov @ 2022-02-01 9:14 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-stable, Michael S . Tsirkin, qemu-devel, Dmitry V . Orekhov,
Marian Postevca
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
^ permalink raw reply [flat|nested] 30+ messages in thread