Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

[Dan Williams]

On Thu, Dec 8, 2016 at 5:59 PM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote:
> Hi, Rafael and Dan
>
>> From: Dan Williams [mailto:dan.j.williams@xxxxxxxxx]
>> Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and
>> early_acpi_os_unmap_memory() from Linux kernel
>>
>> On Thu, Dec 8, 2016 at 5:18 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>> > On Thu, Dec 8, 2016 at 2:11 AM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>> >> On Tue, Nov 29, 2016 at 11:21 PM, Lv Zheng <lv.zheng@xxxxxxxxx> wrote:
>> >>> ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07
>> >>>
>> >>> This patch back ports Linux acpi_get_table_with_size() and
>> >>> early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.
>> >>>
>> >>> The 2 APIs are used by Linux as table management APIs for long time, it
>> >>> contains a hidden logic that during the early stage, the mapped tables
>> >>> should be unmapped before the early stage ends.
>> >>>
>> >>> During the early stage, tables are handled by the following sequence:
>> >>>  acpi_get_table_with_size();
>> >>>  parse the table
>> >>>  early_acpi_os_unmap_memory();
>> >>> During the late stage, tables are handled by the following sequence:
>> >>>  acpi_get_table();
>> >>>  parse the table
>> >>> Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the
>> >>> late stage.
>> >>>
>> >>> The reasoning of introducing acpi_get_table_with_size() is: ACPICA will
>> >>> remember the early mapped pointer in acpi_get_table() and Linux isn't able to
>> >>> prevent ACPICA from using the wrong early mapped pointer during the late
>> >>> stage as there is no API provided from ACPICA to be an inverse of
>> >>> acpi_get_table() to forget the early mapped pointer.
>> >>>
>> >>> But how ACPICA can work with the early/late stage requirement? Inside of
>> >>> ACPICA, tables are ensured to be remained in "INSTALLED" state during the
>> >>> early stage, and they are carefully not transitioned to "VALIDATED" state
>> >>> until the late stage. So the same logic is in fact implemented inside of
>> >>> ACPICA in a different way. The gap is only that the feature is not provided
>> >>> to the OSPMs in an accessible external API style.
>> >>>
>> >>> It then is possible to fix the gap by providing an inverse of
>> >>> acpi_get_table() from ACPICA, so that the two Linux sequences can be
>> >>> combined:
>> >>>  acpi_get_table();
>> >>>  parse the table
>> >>>  acpi_put_table();
>> >>> In order to work easier with the current Linux code, acpi_get_table() and
>> >>> acpi_put_table() is implemented in a usage counting based style:
>> >>>  1. When the usage count of the table is increased from 0 to 1, table is
>> >>>     mapped and .Pointer is set with the mapping address (VALIDATED);
>> >>>  2. When the usage count of the table is decreased from 1 to 0, .Pointer
>> >>>     is unset and the mapping address is unmapped (INVALIDATED).
>> >>> So that we can deploy the new APIs to Linux with minimal effort by just
>> >>> invoking acpi_get_table() in acpi_get_table_with_size() and invoking
>> >>> acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.
>> >>>
>> >>> Link: https://github.com/acpica/acpica/commit/cac67909
>> >>> Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx>
>> >>> Signed-off-by: Bob Moore <robert.moore@xxxxxxxxx>
>> >>
>> >> This commit in -next (071b39575679 ACPICA: Tables: Back port
>> >> acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux
>> >> kernel) causes a regression in my nfit/nvdimm test environment. The
>> >> nfit produced by QEMU no longer results in a nvdimm bus being created.
>> >>
>> >> I have not root caused it, but I'm using the following command line
>> >> options to create an nfit in qemu-2.6.  Reverting the commit leads
>> >> compile failures.
>> >
>> > Would the build problems go away if you reverted "ACPICA: Tables:
>> > Allow FADT to be customized with virtual address" (linux-next commit
>> > cf334d3174f9) in addition to it?
>>
>> Yes, reverting those two commits gets me back to a functional environment:
>>
>> Revert "ACPICA: Tables: Allow FADT to be customized with virtual address"
>> Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and
>> early_acpi_os_un
>
> To Dan:
> It seems in drivers/acpi/nfit/core.c.
> The returned table size is used by the NFIT code.
> I think it should be changed to use table_header->length.

Does the acpi core already validate that table_header->length is
correct? i.e. is is possible that a broken implementation could have
the wrong length in the header? I was assuming that was the purpose of
the _with_size(), but maybe I was wrong?