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

[Tang Chen <tangchen@cn.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

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@cn.fujitsu.com>
>> Reviewed-by: Zhang Yanfei<zhangyanfei@cn.fujitsu.com>
>
> 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.

[-- Attachment #2: Re: [PATCH v2 05_18] x86, acpi: Split acpi_boot_table_init() into two parts..eml --]
[-- Type: message/rfc822, Size: 14802 bytes --]

From: "Zheng, Lv" <lv.zheng@intel.com>
To: Tang Chen <tangchen@cn.fujitsu.com>
Cc: Toshi Kani <toshi.kani@hp.com>, "rjw@sisk.pl" <rjw@sisk.pl>, "lenb@kernel.org" <lenb@kernel.org>, "tglx@linutronix.de" <tglx@linutronix.de>, "mingo@elte.hu" <mingo@elte.hu>, "hpa@zytor.com" <hpa@zytor.com>, "akpm@linux-foundation.org" <akpm@linux-foundation.org>, "tj@kernel.org" <tj@kernel.org>, "trenn@suse.de" <trenn@suse.de>, "yinghai@kernel.org" <yinghai@kernel.org>, "jiang.liu@huawei.com" <jiang.liu@huawei.com>, "wency@cn.fujitsu.com" <wency@cn.fujitsu.com>, "laijs@cn.fujitsu.com" <laijs@cn.fujitsu.com>, "isimatu.yasuaki@jp.fujitsu.com" <isimatu.yasuaki@jp.fujitsu.com>, "izumi.taku@jp.fujitsu.com" <izumi.taku@jp.fujitsu.com>, "mgorman@suse.de" <mgorman@suse.de>, "minchan@kernel.org" <minchan@kernel.org>, "mina86@mina86.com" <mina86@mina86.com>, "gong.chen@linux.intel.com" <gong.chen@linux.intel.com>, "vasilis.liaskovitis@profitbricks.com" <vasilis.liaskovitis@profitbricks.com>, "lwoodman@redhat.com" <lwoodman@redhat.com>, "riel@redhat.com" <riel@redhat.com>, "jweiner@redhat.com" <jweiner@redhat.com>, "prarit@redhat.com" <prarit@redhat.com>, "zhangyanfei@cn.fujitsu.com" <zhangyanfei@cn.fujitsu.com>, "yanghy@cn.fujitsu.com" <yanghy@cn.fujitsu.com>, "x86@kernel.org" <x86@kernel.org>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, "Moore, Robert" <robert.moore@intel.com>
Subject: RE: [PATCH v2 05/18] x86, acpi: Split acpi_boot_table_init() into two parts.
Date: Fri, 2 Aug 2013 08:11:15 +0000
Message-ID: <1AE640813FDE7649BE1B193DEA596E8802437C27@SHSMSX101.ccr.corp.intel.com>

> 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.

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.