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.