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
> >  
>