On Fri, Feb 26, 2021 at 11:16:06AM -0500, George Kennedy wrote:
> On 2/26/2021 6:17 AM, Mike Rapoport wrote:
> > Hi George,
> >
> > On Thu, Feb 25, 2021 at 08:19:18PM -0500, George Kennedy wrote:
> > >
> > > Not sure if it's the right thing to do, but added
> > > "acpi_tb_find_table_address()" to return the physical address of a table to
> > > use with memblock_reserve().
> > >
> > > virt_to_phys(table) does not seem to return the physical address for the
> > > iBFT table (it would be nice if struct acpi_table_header also had a
> > > "address" element for the physical address of the table).
> >
> > virt_to_phys() does not work that early because then it is mapped with
> > early_memremap() which uses different virtual to physical scheme.
> >
> > I'd say that acpi_tb_find_table_address() makes sense if we'd like to
> > reserve ACPI tables outside of drivers/acpi.
> >
> > But probably we should simply reserve all the tables during
> > acpi_table_init() so that any table that firmware put in the normal memory
> > will be surely reserved.
> > > Ran 10 successful boots with the above without failure.
> > That's good news indeed :)
>
> Wondering if we could do something like this instead (trying to keep changes
> minimal). Just do the memblock_reserve() for all the standard tables.
I think something like this should work, but I'm not an ACPI expert to say
if this the best way to reserve the tables.
> diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
> index 0bb15ad..830f82c 100644
> --- a/drivers/acpi/acpica/tbinstal.c
> +++ b/drivers/acpi/acpica/tbinstal.c
> @@ -7,6 +7,7 @@
> *
> *****************************************************************************/
>
> +#include <linux/memblock.h>
> #include <acpi/acpi.h>
> #include "accommon.h"
> #include "actables.h"
> @@ -14,6 +15,23 @@
> #define _COMPONENT ACPI_TABLES
> ACPI_MODULE_NAME("tbinstal")
>
> +void
> +acpi_tb_reserve_standard_table(acpi_physical_address address,
> + struct acpi_table_header *header)
> +{
> + struct acpi_table_header local_header;
> +
> + if ((ACPI_COMPARE_NAMESEG(header->signature, ACPI_SIG_FACS)) ||
> + (ACPI_VALIDATE_RSDP_SIG(header->signature))) {
> + return;
> + }
> + /* Standard ACPI table with full common header */
> +
> + memcpy(&local_header, header, sizeof(struct acpi_table_header));
> +
> + memblock_reserve(address, PAGE_ALIGN(local_header.length));
> +}
> +
> /*******************************************************************************
> *
> * FUNCTION: acpi_tb_install_table_with_override
> @@ -58,6 +76,9 @@
> new_table_desc->flags,
> new_table_desc->pointer);
>
> + acpi_tb_reserve_standard_table(new_table_desc->address,
> + new_table_desc->pointer);
> +
> acpi_tb_print_table_header(new_table_desc->address,
> new_table_desc->pointer);
>
> There should be no harm in doing the memblock_reserve() for all the standard
> tables, right?
It should be ok to memblock_reserve() all the tables very early as long as
we don't run out of static entries in memblock.reserved.
We just need to make sure the tables are reserved before memblock
allocations are possible, so we'd still need to move acpi_table_init() in
x86::setup_arch() before e820__memblock_setup().
Not sure how early ACPI is initialized on arm64.
> Ran 10 boots with the above without failure.
>
> George
--
Sincerely yours,
Mike.
