Re: [PATCH v2 RESEND 07/18] x86, ACPI: Also initialize signatureand length when parsing root table.

[Tang Chen]

Hi Rafael,

On 08/02/2013 09:03 PM, Rafael J. Wysocki wrote:
> On Friday, August 02, 2013 05:14:26 PM Tang Chen wrote:
> > Besides the phys addr of the acpi tables, it will be very convenient if
> > we also have the signature of each table in acpi_gbl_root_table_list at
> > early time. We can find SRAT easily by comparing the signature.
> >
> > This patch alse record signature and some other info in
> > acpi_gbl_root_table_list at early time.
> >
> > Signed-off-by: Tang Chen<tangchen@xxxxxxxxxxxxxx>
> > Reviewed-by: Zhang Yanfei<zhangyanfei@xxxxxxxxxxxxxx>
>
> The subject is misleading, as the change is in ACPICA and therefore affects not
> only x86.

OK, will change it.

> Also I think the same comments as for the other ACPICA patch is this series
> applies: You shouldn't modify acpi_tbl_parse_root_table() in ways that would
> require the other OSes using ACPICA to be modified.

Thank you for the reminding. Please refer to the attachment.
How do you think of the idea from Zheng ?

Thanks.
> --- Begin Message ---
> > From: Tang Chen [mailto:tangchen@xxxxxxxxxxxxxx]
> > 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.
>
> 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.
>
> --- End Message ---