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.
So it's fine you want to put acpi_early_init earlier.
>
> 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.
>
> 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.f...@cn.fujitsu.com]
> > > > > Sent: Friday, July 14, 2017 1:53 PM
> > > > > To: x...@kernel.org; linux-kernel@vger.kernel.org
> > > > > Cc: t...@linutronix.de; mi...@kernel.org; h...@zytor.com;
> > > > > ebied...@xmission.com; b...@redhat.com;
> > > > > pet...@infradead.org; izumi.t...@jp.fujitsu.com;
> > > > > tokunaga.kei...@jp.fujitsu.com; Dou Liyang
> > > > > <douly.f...@cn.fujitsu.com>; linux-a...@vger.kernel.org; Rafael J.
> > > > > Wysocki <r...@rjwysocki.net>; Zheng,
> > > > > Lv <lv.zh...@intel.com>; Julian Wollrath <jwollr...@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...@intel.com>
> > > > > Signed-off-by: Dou Liyang <douly.f...@cn.fujitsu.com>
> > > > > Cc: linux-a...@vger.kernel.org
> > > > > Cc: Rafael J. Wysocki <r...@rjwysocki.net>
> > > > > Cc: Zheng, Lv <lv.zh...@intel.com>
> > > > > Cc: Julian Wollrath <jwollr...@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
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> >
> >
> >
>
>
