On Tue, 1 Feb 2022 13:25:20 +0530 (IST) Ani Sinha <a...@anisinha.ca> wrote:
> On Tue, 1 Feb 2022, Igor Mammedov wrote: > > > On Mon, 31 Jan 2022 19:51:24 +0530 (IST) > > Ani Sinha <a...@anisinha.ca> wrote: > > > > > On Mon, 31 Jan 2022, Igor Mammedov wrote: > > > > > > > On Mon, 31 Jan 2022 18:58:57 +0530 (IST) > > > > Ani Sinha <a...@anisinha.ca> wrote: > > > > > > > > > On Mon, 31 Jan 2022, Igor Mammedov wrote: > > > > > > > > > > > On Mon, 31 Jan 2022 11:47:00 +0530 > > > > > > Ani Sinha <a...@anisinha.ca> wrote: > > > > > > > > > > > > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov > > > > > > > <imamm...@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
