RE: [PATCH v2 05/18] x86, acpi: Split acpi_boot_table_init() into two parts.

["Zheng, Lv" <lv.zheng@intel.com>]

> From: Zheng, Lv
> Sent: Friday, August 02, 2013 4:11 PM
> 
> > From: Tang Chen [mailto:tangchen@cn.fujitsu.com]
> > Sent: Friday, August 02, 2013 3:01 PM
> >
> > On 08/02/2013 01:25 PM, Zheng, Lv wrote:
> > ......
> > >>> index ce3d5db..9d68ffc 100644
> > >>> --- a/drivers/acpi/acpica/tbutils.c
> > >>> +++ b/drivers/acpi/acpica/tbutils.c
> > >>> @@ -766,9 +766,30 @@
> > >> acpi_tb_parse_root_table(acpi_physical_address rsdp_address)
> > >>>   	*/
> > >>>   	acpi_os_unmap_memory(table, length);
> > >>>
> > >>> +	return_ACPI_STATUS(AE_OK);
> > >>> +}
> > >>> +
> > >>>
> > >
> > > I don't think you can split the function here.
> > > ACPICA still need to continue to parse the table using the logic
> > implemented in the acpi_tb_install_table() and acpi_tb_parse_fadt().
> > (for example, endianess of the signature).
> > > You'd better to keep them as is and split some codes from
> > 'acpi_tb_install_table' to form another function:
> > acpi_tb_override_table().
> >
> > I'm sorry, I don't quite follow this.
> >
> > I split acpi_tb_parse_root_table(), not acpi_tb_install_table() and
> > acpi_tb_parse_fadt().
> > If ACPICA wants to use these two functions somewhere else, I think it
> is
> > OK, isn't it?
> >
> > And the reason I did this, please see below.
> >
> > ......
> > >>> + *
> > >>> + * FUNCTION:    acpi_tb_install_root_table
> > >
> > > I think this function should be acpi_tb_override_tables, and call
> > acpi_tb_override_table() inside this function for each table.
> >
> > It is not just about acpi initrd table override.
> >
> > acpi_tb_parse_root_table() was split into two steps:
> > 1. initialize acpi_gbl_root_table_list
> > 2. install tables into acpi_gbl_root_table_list
> >
> > I need step1 earlier because I want to find SRAT at early time.
> > But I don't want step2 earlier because before install the tables in
> > firmware,
> > acpi initrd table override could happen. I want only SRAT, I don't want
> to
> > touch much existing code.
> 
> According to what you've explained, what you didn’t want to be called
> earlier is exactly "acpi initrd table override", please split only this logic to
> the step 2 and leave the others remained.
> I think you should write a function named as acpi_override_tables() or
> likewise in tbxface.c to be executed as the OSPM entry of the step 2.
> Inside this function, acpi_tb_table_override() should be called.
> 
> 268 void
> 269 acpi_tb_install_table(acpi_physical_address address,
> 270                       char *signature, u32 table_index)
> 271 {
> 
> I think you still need the following codes to be called at the early stage.
> 
> 272         struct acpi_table_header *table;
> 273         struct acpi_table_header *final_table;
> 274         struct acpi_table_desc *table_desc;
> 275
> 276         if (!address) {
> 277                 ACPI_ERROR((AE_INFO,
> 278                             "Null physical address for ACPI
> table [%s]",
> 279                             signature));
> 280                 return;
> 281         }
> 282
> 283         /* Map just the table header */
> 284
> 285         table = acpi_os_map_memory(address, sizeof(struct
> acpi_table_header));
> 286         if (!table) {
> 287                 ACPI_ERROR((AE_INFO,
> 288                             "Could not map memory for
> table [%s] at %p",
> 289                             signature, ACPI_CAST_PTR(void,
> address)));
> 290                 return;
> 291         }
> 292
> 293         /* If a particular signature is expected (DSDT/FACS), it
> must match */
> 294
> 295         if (signature
> && !ACPI_COMPARE_NAME(table->signature, signature)) {
> 296                 ACPI_BIOS_ERROR((AE_INFO,
> 297                                  "Invalid signature 0x%X for
> ACPI table, expected [%s]",
> 298                                  *ACPI_CAST_PTR(u32,
> table->signature),
> 299                                  signature));
> 300                 goto unmap_and_exit;
> 301         }
> 302
> 303         /*
> 304          * Initialize the table entry. Set the pointer to NULL, since
> the
> 305          * table is not fully mapped at this time.
> 306          */
> 307         table_desc =
> &acpi_gbl_root_table_list.tables[table_index];
> 308
> 309         table_desc->address = address;
> 310         table_desc->pointer = NULL;
> 311         table_desc->length = table->length;
> 312         table_desc->flags = ACPI_TABLE_ORIGIN_MAPPED;
> 313         ACPI_MOVE_32_TO_32(table_desc->signature.ascii,
> table->signature);
> 314
> 
> You should delete the following codes:
> 
> 315         /*
> 316          * ACPI Table Override:
> 317          *
> 318          * Before we install the table, let the host OS override it
> with a new
> 319          * one if desired. Any table within the RSDT/XSDT can be
> replaced,
> 320          * including the DSDT which is pointed to by the FADT.
> 321          *
> 322          * NOTE: If the table is overridden, then final_table will
> contain a
> 323          * mapped pointer to the full new table. If the table is not
> overridden,
> 324          * or if there has been a physical override, then the table
> will be
> 325          * fully mapped later (in verify table). In any case, we
> must
> 326          * unmap the header that was mapped above.
> 327          */
> 328         final_table = acpi_tb_table_override(table, table_desc);
> 329         if (!final_table) {
> 330                 final_table = table;    /* There was no
> override */
> 331         }
> 332
> 
> You still need to keep the following logic.
> 
> 333         acpi_tb_print_table_header(table_desc->address,
> final_table);
> 334
> 335         /* Set the global integer width (based upon revision of the
> DSDT) */
> 336
> 337         if (table_index == ACPI_TABLE_INDEX_DSDT) {
> 338
> acpi_ut_set_integer_width(final_table->revision);
> 339         }
> 340
> 
> You should delete the following codes:
> 
> 341         /*
> 342          * If we have a physical override during this early loading
> of the ACPI
> 343          * tables, unmap the table for now. It will be mapped
> again later when
> 344          * it is actually used. This supports very early loading of
> ACPI tables,
> 345          * before virtual memory is fully initialized and running
> within the
> 346          * host OS. Note: A logical override has the
> ACPI_TABLE_ORIGIN_OVERRIDE
> 347          * flag set and will not be deleted below.
> 348          */
> 349         if (final_table != table) {
> 350                 acpi_tb_delete_table(table_desc);
> 351         }
> 
> Keep the following.
> 
> 352
> 353       unmap_and_exit:
> 354
> 355         /* Always unmap the table header that we mapped above
> */
> 356
> 357         acpi_os_unmap_memory(table, sizeof(struct
> acpi_table_header));
> 358 }
> 
> I'm not sure if this can make my concerns clearer for you now.

You might have concerns about how to handle FADT.

In acpi_override_tables, your codes might be looking like:

584         for (i = 0; i < acpi_gbl_root_table_list.current_table_count; i++) {

Just change the I from 2 to 0.

585                 acpi_tb_table_override (...);
595         }

You don't need to call acpi_tb_parse_table, the tables pointed by the FADT are DSDT and FACS, their index is 0 and 1.

Thanks and best regards
-Lv

> 
> Thanks and best regards
> -Lv
> 
> >
> > Would you please explain more about your comment ? I think maybe I
> > missed something
> > important to you guys. :)
> >
> > And all the other ACPICA rules will be followed in the next version.
> >
> > Thanks.