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.