lkml.org 
[lkml]   [2017]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 12/13] ACPI / init: Invoke early ACPI initialization earlier
From
Date
Hi Baoquan,

At 07/27/2017 02:08 PM, bhe@redhat.com wrote:
> On 07/26/17 at 08:19pm, Dou Liyang wrote:
>> Hi Baoquan,
>>>> There are two places where we used DMAR table in Linux:
>>>>
>>>> 1) In detect_intel_iommu() in ACPI early stage:
>>>>
>>>> ...
>>>> status = acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_tbl);
>>>> ....
>>>> if (dmar_tbl) {
>>>> acpi_put_table(dmar_tbl);
>>>> dmar_tbl = NULL;
>>>> }
>>>>
>>>> 2) In dmar_table_init() in ACPI late stage:
>>>>
>>>> ...
>>>> status = acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_tbl);
>>>> ...
>>>>
>>>> As we know, dmar_table_init() is called by intel_iommu_init() and
>>>> intel_prepare_irq_remapping().
>>>>
>>>> When I invoked acpi_put_table() in the intel_prepare_irq_remapping() in
>>>> early stage like 1) shows, kernel will panic.
>>>
>>> That's because acpi_put_table() will make the table pointer be NULL,
>>> while dmar_table_init() will skip parse_dmar_table() calling if
>>> dmar_table_initialized is set to 1 in intel_prepare_irq_remapping().
>>>
>>> Dmar hardware support interrupt remapping and io remapping separately. But
>>> intel_iommu_init() is called later than intel_prepare_irq_remapping().
>>> So what if make dmar_table_init() a reentrant function? You can just
>>> have a try, but maybe not a good idea, the dmar table will be parsed
>>> twice.
>>
>> The true reason why the kernel panic is that acpi_put_table() only
>> released DMAR table structure, but not released the remapping
>> structures in DMAR table, such as DRHD, RMRR. So the address of
>> RMRR parsed in early ACPI stage will be used in late ACPI stage in
>> intel_iommu_init(), which make the kernel panic.
>>
>> The solution is invoking the intel_iommu_free_dmars() before
>> dmar_table_init() in intel_iommu_init() to release the RMRR.
>> Demo code will show at the bottom.
>>
>> I prefer to invoke acpi_early_init() earlier. But it needs a regression
>> test[1].
>
> Good work, in fact not only intel iommu matters here, I gues you haven't
> tried amd system with a AMD-VI which does the amd iommu work. It's
> similar to intel iommu and the same principle but different acpi tables.

Yes, I missed it, I didn't try it in AMD platform. If I want to use
acpi_put_table(), I should consider both the Intel and AMD platform.

> So it's fine you want to put acpi_early_init earlier.

Got it.

>
>>
>> I am looking for Thinkpad x121e (AMD E-450 APU) to test. I have tested
>> it in Thinkpad s430, It's OK.
>>
>> BTY, I am confused how does the ACPI subsystem affect PIT which
>> will be used to fast calibrate CPU frequency[2].
>
> I checked code, and have no any idea. Add Joey Lee to list, see if he
> can help answer this.

Thank you very much.

sincerely,

dou.

>
>>
>> Do you have any idea?
>>
>> [1] https://lkml.org/lkml/2014/3/10/123
>> [2] https://lkml.org/lkml/2014/3/12/3
>>
>>
>> drivers/iommu/dmar.c | 27 +++++++++++----------------
>> drivers/iommu/intel-iommu.c | 2 ++
>> drivers/iommu/intel_irq_remapping.c | 17 ++++++++++++++++-
>> include/linux/dmar.h | 2 ++
>> init/main.c | 2 +-
>> 5 files changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
>> index c8b0329..e6261b7 100644
>> --- a/drivers/iommu/dmar.c
>> +++ b/drivers/iommu/dmar.c
>> @@ -68,6 +68,8 @@ DECLARE_RWSEM(dmar_global_lock);
>> LIST_HEAD(dmar_drhd_units);
>>
>> struct acpi_table_header * __initdata dmar_tbl;
>> +struct acpi_table_header * __initdata dmar_tbl_original;
>> +
>> static int dmar_dev_scope_status = 1;
>> static unsigned long dmar_seq_ids[BITS_TO_LONGS(DMAR_UNITS_SUPPORTED)];
>>
>> @@ -627,6 +629,7 @@ parse_dmar_table(void)
>> * fixed map.
>> */
>> dmar_table_detect();
>> + dmar_tbl_original = dmar_tbl;
>>
>> /*
>> * ACPI tables may not be DMA protected by tboot, so use DMAR copy
>> @@ -811,26 +814,18 @@ int __init dmar_dev_scope_init(void)
>>
>> int __init dmar_table_init(void)
>> {
>> - static int dmar_table_initialized;
>> int ret;
>>
>> - if (dmar_table_initialized == 0) {
>> - ret = parse_dmar_table();
>> - if (ret < 0) {
>> - if (ret != -ENODEV)
>> - pr_info("Parse DMAR table failure.\n");
>> - } else if (list_empty(&dmar_drhd_units)) {
>> - pr_info("No DMAR devices found\n");
>> - ret = -ENODEV;
>> - }
>> -
>> - if (ret < 0)
>> - dmar_table_initialized = ret;
>> - else
>> - dmar_table_initialized = 1;
>> + ret = parse_dmar_table();
>> + if (ret < 0) {
>> + if (ret != -ENODEV)
>> + pr_info("Parse DMAR table failure.\n");
>> + } else if (list_empty(&dmar_drhd_units)) {
>> + pr_info("No DMAR devices found\n");
>> + ret = -ENODEV;
>> }
>>
>> - return dmar_table_initialized < 0 ? dmar_table_initialized : 0;
>> + return ret;
>> }
>>
>> static void warn_invalid_dmar(u64 addr, const char *message)
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 687f18f..90f74f4 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -4832,6 +4832,8 @@ int __init intel_iommu_init(void)
>> }
>>
>> down_write(&dmar_global_lock);
>> +
>> + intel_iommu_free_dmars();
>> if (dmar_table_init()) {
>> if (force_on)
>> panic("tboot: Failed to initialize DMAR table\n");
>> diff --git a/drivers/iommu/intel_irq_remapping.c
>> b/drivers/iommu/intel_irq_remapping.c
>> index a5b89f6..ccaacda 100644
>> --- a/drivers/iommu/intel_irq_remapping.c
>> +++ b/drivers/iommu/intel_irq_remapping.c
>> @@ -675,7 +675,7 @@ static void __init intel_cleanup_irq_remapping(void)
>> pr_warn("Failed to enable irq remapping. You are vulnerable to
>> irq-injection attacks.\n");
>> }
>>
>> -static int __init intel_prepare_irq_remapping(void)
>> +static int __init __intel_prepare_irq_remapping(void)
>> {
>> struct dmar_drhd_unit *drhd;
>> struct intel_iommu *iommu;
>> @@ -743,6 +743,21 @@ static int __init intel_prepare_irq_remapping(void)
>> return -ENODEV;
>> }
>>
>> +static int __init intel_prepare_irq_remapping(void)
>> +{
>> + int ret;
>> +
>> + ret = __intel_prepare_irq_remapping();
>> +
>> + if (dmar_tbl_original) {
>> + acpi_put_table(dmar_tbl_original);
>> + dmar_tbl_original = NULL;
>> + dmar_tbl = NULL;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> /*
>> * Set Posted-Interrupts capability.
>> */
>> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
>> index e8ffba1..987b076 100644
>> --- a/include/linux/dmar.h
>> +++ b/include/linux/dmar.h
>> @@ -50,6 +50,8 @@ struct dmar_dev_scope {
>>
>> #ifdef CONFIG_DMAR_TABLE
>> extern struct acpi_table_header *dmar_tbl;
>> +extern struct acpi_table_header *dmar_tbl_original;
>> +
>> struct dmar_drhd_unit {
>> struct list_head list; /* list of drhd units */
>> struct acpi_dmar_header *hdr; /* ACPI header */
>> diff --git a/init/main.c b/init/main.c
>> index 52dee20..052481f 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -655,12 +655,12 @@ asmlinkage __visible void __init start_kernel(void)
>> kmemleak_init();
>> setup_per_cpu_pageset();
>> numa_policy_init();
>> - acpi_early_init();
>> if (late_time_init)
>> late_time_init();
>> calibrate_delay();
>> pidmap_init();
>> anon_vma_init();
>> + acpi_early_init();
>> #ifdef CONFIG_X86
>> if (efi_enabled(EFI_RUNTIME_SERVICES))
>> efi_enter_virtual_mode();
>>
>> Thanks,
>> dou.
>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> dou.
>>>>>
>>>>> Thanks
>>>>> Lv
>>>>>
>>>>>> From: Dou Liyang [mailto:douly.fnst@cn.fujitsu.com]
>>>>>> Sent: Friday, July 14, 2017 1:53 PM
>>>>>> To: x86@kernel.org; linux-kernel@vger.kernel.org
>>>>>> Cc: tglx@linutronix.de; mingo@kernel.org; hpa@zytor.com; ebiederm@xmission.com; bhe@redhat.com;
>>>>>> peterz@infradead.org; izumi.taku@jp.fujitsu.com; tokunaga.keiich@jp.fujitsu.com; Dou Liyang
>>>>>> <douly.fnst@cn.fujitsu.com>; linux-acpi@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>; Zheng,
>>>>>> Lv <lv.zheng@intel.com>; Julian Wollrath <jwollrath@web.de>
>>>>>> Subject: [PATCH v7 12/13] ACPI / init: Invoke early ACPI initialization earlier
>>>>>>
>>>>>> Linux uses acpi_early_init() to put the ACPI table management into
>>>>>> the late stage from the early stage where the mapped ACPI tables is
>>>>>> temporary and should be unmapped.
>>>>>>
>>>>>> But, now initializing interrupt delivery mode should map and parse the
>>>>>> DMAR table earlier in the early stage. This causes an ACPI error when
>>>>>> Linux reallocates the ACPI root tables. Because Linux doesn't unmapped
>>>>>> the DMAR table after using in the early stage.
>>>>>>
>>>>>> Invoke acpi_early_init() earlier before late_time_init(), Keep the DMAR
>>>>>> be mapped and parsed in late stage like before.
>>>>>>
>>>>>> Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>>>>>> Cc: linux-acpi@vger.kernel.org
>>>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>>>>> Cc: Zheng, Lv <lv.zheng@intel.com>
>>>>>> Cc: Julian Wollrath <jwollrath@web.de>
>>>>>> ---
>>>>>> Test in my own PC(Lenovo M4340).
>>>>>> Ask help for doing regression testing for the bug said in commit c4e1acbb35e4
>>>>>> ("ACPI / init: Invoke early ACPI initialization later").
>>>>>>
>>>>>> init/main.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/init/main.c b/init/main.c
>>>>>> index df58a41..7a09467 100644
>>>>>> --- a/init/main.c
>>>>>> +++ b/init/main.c
>>>>>> @@ -654,12 +654,12 @@ asmlinkage __visible void __init start_kernel(void)
>>>>>> kmemleak_init();
>>>>>> setup_per_cpu_pageset();
>>>>>> numa_policy_init();
>>>>>> + acpi_early_init();
>>>>>> if (late_time_init)
>>>>>> late_time_init();
>>>>>> calibrate_delay();
>>>>>> pidmap_init();
>>>>>> anon_vma_init();
>>>>>> - acpi_early_init();
>>>>>> #ifdef CONFIG_X86
>>>>>> if (efi_enabled(EFI_RUNTIME_SERVICES))
>>>>>> efi_enter_virtual_mode();
>>>>>> --
>>>>>> 2.5.5
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>


\
 
 \ /
  Last update: 2017-07-27 08:30    [W:0.245 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site