* Re: [PATCH v2] iommu/vt-d: Turn off translations at shutdown
2019-11-10 17:27 [PATCH v2] iommu/vt-d: Turn off translations at shutdown Deepa Dinamani
@ 2019-11-11 1:32 ` Lu Baolu
2019-11-11 3:21 ` Deepa Dinamani
2019-11-11 5:00 ` Lu Baolu
2019-11-11 15:08 ` Joerg Roedel
2 siblings, 1 reply; 6+ messages in thread
From: Lu Baolu @ 2019-11-11 1:32 UTC (permalink / raw)
To: Deepa Dinamani, linux-kernel, joro; +Cc: baolu.lu, dwmw2, iommu
Hi,
On 11/11/19 1:27 AM, Deepa Dinamani wrote:
> The intel-iommu driver assumes that the iommu state is
> cleaned up at the start of the new kernel.
> But, when we try to kexec boot something other than the
> Linux kernel, the cleanup cannot be relied upon.
> Hence, cleanup before we go down for reboot.
>
> Keeping the cleanup at initialization also, in case BIOS
> leaves the IOMMU enabled.
Do you mind shining more light on this statement? I can't get your point
here. Currently iommu_shutdown() only happens for reboot, right?
Best regards,
baolu
>
> I considered turning off iommu only during kexec reboot, but a clean
> shutdown seems always a good idea. But if someone wants to make it
> conditional, such as VMM live update, we can do that. There doesn't
> seem to be such a condition at this time.
>
> Tested that before, the info message
> 'DMAR: Translation was enabled for <iommu> but we are not in kdump mode'
> would be reported for each iommu. The message will not appear when the
> DMA-remapping is not enabled on entry to the kernel.
>
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> ---
> Changes since v1:
> * move shutdown registration to iommu detection
>
> drivers/iommu/dmar.c | 5 ++++-
> drivers/iommu/intel-iommu.c | 20 ++++++++++++++++++++
> include/linux/dmar.h | 2 ++
> 3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index eecd6a421667..3acfa6a25fa2 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -895,8 +895,11 @@ int __init detect_intel_iommu(void)
> }
>
> #ifdef CONFIG_X86
> - if (!ret)
> + if (!ret) {
> x86_init.iommu.iommu_init = intel_iommu_init;
> + x86_platform.iommu_shutdown = intel_iommu_shutdown;
> + }
> +
> #endif
>
> if (dmar_tbl) {
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index fe8097078669..7ac73410ba8e 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4764,6 +4764,26 @@ static void intel_disable_iommus(void)
> iommu_disable_translation(iommu);
> }
>
> +void intel_iommu_shutdown(void)
> +{
> + struct dmar_drhd_unit *drhd;
> + struct intel_iommu *iommu = NULL;
> +
> + if (no_iommu || dmar_disabled)
> + return;
> +
> + down_write(&dmar_global_lock);
> +
> + /* Disable PMRs explicitly here. */
> + for_each_iommu(iommu, drhd)
> + iommu_disable_protect_mem_regions(iommu);
> +
> + /* Make sure the IOMMUs are switched off */
> + intel_disable_iommus();
> +
> + up_write(&dmar_global_lock);
> +}
> +
> static inline struct intel_iommu *dev_to_intel_iommu(struct device *dev)
> {
> struct iommu_device *iommu_dev = dev_to_iommu_device(dev);
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index a7cf3599d9a1..f64ca27dc210 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -129,6 +129,7 @@ static inline int dmar_res_noop(struct acpi_dmar_header *hdr, void *arg)
> #ifdef CONFIG_INTEL_IOMMU
> extern int iommu_detected, no_iommu;
> extern int intel_iommu_init(void);
> +extern void intel_iommu_shutdown(void);
> extern int dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg);
> extern int dmar_parse_one_atsr(struct acpi_dmar_header *header, void *arg);
> extern int dmar_check_one_atsr(struct acpi_dmar_header *hdr, void *arg);
> @@ -137,6 +138,7 @@ extern int dmar_iommu_hotplug(struct dmar_drhd_unit *dmaru, bool insert);
> extern int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info);
> #else /* !CONFIG_INTEL_IOMMU: */
> static inline int intel_iommu_init(void) { return -ENODEV; }
> +static inline void intel_iommu_shutdown(void) { }
>
> #define dmar_parse_one_rmrr dmar_res_noop
> #define dmar_parse_one_atsr dmar_res_noop
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] iommu/vt-d: Turn off translations at shutdown
2019-11-11 1:32 ` Lu Baolu
@ 2019-11-11 3:21 ` Deepa Dinamani
2019-11-11 4:58 ` Lu Baolu
0 siblings, 1 reply; 6+ messages in thread
From: Deepa Dinamani @ 2019-11-11 3:21 UTC (permalink / raw)
To: Lu Baolu; +Cc: Linux Kernel Mailing List, joro, David Woodhouse, iommu
On Sun, Nov 10, 2019 at 5:35 PM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> Hi,
>
> On 11/11/19 1:27 AM, Deepa Dinamani wrote:
> > The intel-iommu driver assumes that the iommu state is
> > cleaned up at the start of the new kernel.
> > But, when we try to kexec boot something other than the
> > Linux kernel, the cleanup cannot be relied upon.
> > Hence, cleanup before we go down for reboot.
> >
> > Keeping the cleanup at initialization also, in case BIOS
> > leaves the IOMMU enabled.
>
> Do you mind shining more light on this statement? I can't get your point
> here. Currently iommu_shutdown() only happens for reboot, right?
In this part, I'm saying that I'm leaving intel_iommu_init() alone.
I'm not taking out disabling the iommu from intel_iommu_init(). This
will help when BIOS has enabled the IOMMU and for whatever reason, the
kernel is booting with IOMMU off.
-Deepa
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iommu/vt-d: Turn off translations at shutdown
2019-11-11 3:21 ` Deepa Dinamani
@ 2019-11-11 4:58 ` Lu Baolu
0 siblings, 0 replies; 6+ messages in thread
From: Lu Baolu @ 2019-11-11 4:58 UTC (permalink / raw)
To: Deepa Dinamani
Cc: baolu.lu, Linux Kernel Mailing List, joro, David Woodhouse, iommu
Hi,
On 11/11/19 11:21 AM, Deepa Dinamani wrote:
> On Sun, Nov 10, 2019 at 5:35 PM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi,
>>
>> On 11/11/19 1:27 AM, Deepa Dinamani wrote:
>>> The intel-iommu driver assumes that the iommu state is
>>> cleaned up at the start of the new kernel.
>>> But, when we try to kexec boot something other than the
>>> Linux kernel, the cleanup cannot be relied upon.
>>> Hence, cleanup before we go down for reboot.
>>>
>>> Keeping the cleanup at initialization also, in case BIOS
>>> leaves the IOMMU enabled.
>>
>> Do you mind shining more light on this statement? I can't get your point
>> here. Currently iommu_shutdown() only happens for reboot, right?
>
> In this part, I'm saying that I'm leaving intel_iommu_init() alone.
> I'm not taking out disabling the iommu from intel_iommu_init(). This
> will help when BIOS has enabled the IOMMU and for whatever reason, the
> kernel is booting with IOMMU off.
Okay, thanks for the explanation.
>
> -Deepa
>
Best regards,
baolu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iommu/vt-d: Turn off translations at shutdown
2019-11-10 17:27 [PATCH v2] iommu/vt-d: Turn off translations at shutdown Deepa Dinamani
2019-11-11 1:32 ` Lu Baolu
@ 2019-11-11 5:00 ` Lu Baolu
2019-11-11 15:08 ` Joerg Roedel
2 siblings, 0 replies; 6+ messages in thread
From: Lu Baolu @ 2019-11-11 5:00 UTC (permalink / raw)
To: Deepa Dinamani, linux-kernel, joro; +Cc: baolu.lu, dwmw2, iommu
Hi,
On 11/11/19 1:27 AM, Deepa Dinamani wrote:
> The intel-iommu driver assumes that the iommu state is
> cleaned up at the start of the new kernel.
> But, when we try to kexec boot something other than the
> Linux kernel, the cleanup cannot be relied upon.
> Hence, cleanup before we go down for reboot.
>
> Keeping the cleanup at initialization also, in case BIOS
> leaves the IOMMU enabled.
>
> I considered turning off iommu only during kexec reboot, but a clean
> shutdown seems always a good idea. But if someone wants to make it
> conditional, such as VMM live update, we can do that. There doesn't
> seem to be such a condition at this time.
>
> Tested that before, the info message
> 'DMAR: Translation was enabled for <iommu> but we are not in kdump mode'
> would be reported for each iommu. The message will not appear when the
> DMA-remapping is not enabled on entry to the kernel.
>
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
This patch looks good to me.
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
Best regards,
baolu
> ---
> Changes since v1:
> * move shutdown registration to iommu detection
>
> drivers/iommu/dmar.c | 5 ++++-
> drivers/iommu/intel-iommu.c | 20 ++++++++++++++++++++
> include/linux/dmar.h | 2 ++
> 3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index eecd6a421667..3acfa6a25fa2 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -895,8 +895,11 @@ int __init detect_intel_iommu(void)
> }
>
> #ifdef CONFIG_X86
> - if (!ret)
> + if (!ret) {
> x86_init.iommu.iommu_init = intel_iommu_init;
> + x86_platform.iommu_shutdown = intel_iommu_shutdown;
> + }
> +
> #endif
>
> if (dmar_tbl) {
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index fe8097078669..7ac73410ba8e 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4764,6 +4764,26 @@ static void intel_disable_iommus(void)
> iommu_disable_translation(iommu);
> }
>
> +void intel_iommu_shutdown(void)
> +{
> + struct dmar_drhd_unit *drhd;
> + struct intel_iommu *iommu = NULL;
> +
> + if (no_iommu || dmar_disabled)
> + return;
> +
> + down_write(&dmar_global_lock);
> +
> + /* Disable PMRs explicitly here. */
> + for_each_iommu(iommu, drhd)
> + iommu_disable_protect_mem_regions(iommu);
> +
> + /* Make sure the IOMMUs are switched off */
> + intel_disable_iommus();
> +
> + up_write(&dmar_global_lock);
> +}
> +
> static inline struct intel_iommu *dev_to_intel_iommu(struct device *dev)
> {
> struct iommu_device *iommu_dev = dev_to_iommu_device(dev);
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index a7cf3599d9a1..f64ca27dc210 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -129,6 +129,7 @@ static inline int dmar_res_noop(struct acpi_dmar_header *hdr, void *arg)
> #ifdef CONFIG_INTEL_IOMMU
> extern int iommu_detected, no_iommu;
> extern int intel_iommu_init(void);
> +extern void intel_iommu_shutdown(void);
> extern int dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg);
> extern int dmar_parse_one_atsr(struct acpi_dmar_header *header, void *arg);
> extern int dmar_check_one_atsr(struct acpi_dmar_header *hdr, void *arg);
> @@ -137,6 +138,7 @@ extern int dmar_iommu_hotplug(struct dmar_drhd_unit *dmaru, bool insert);
> extern int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info);
> #else /* !CONFIG_INTEL_IOMMU: */
> static inline int intel_iommu_init(void) { return -ENODEV; }
> +static inline void intel_iommu_shutdown(void) { }
>
> #define dmar_parse_one_rmrr dmar_res_noop
> #define dmar_parse_one_atsr dmar_res_noop
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] iommu/vt-d: Turn off translations at shutdown
2019-11-10 17:27 [PATCH v2] iommu/vt-d: Turn off translations at shutdown Deepa Dinamani
2019-11-11 1:32 ` Lu Baolu
2019-11-11 5:00 ` Lu Baolu
@ 2019-11-11 15:08 ` Joerg Roedel
2 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2019-11-11 15:08 UTC (permalink / raw)
To: Deepa Dinamani; +Cc: linux-kernel, baolu.lu, dwmw2, iommu
On Sun, Nov 10, 2019 at 09:27:44AM -0800, Deepa Dinamani wrote:
> The intel-iommu driver assumes that the iommu state is
> cleaned up at the start of the new kernel.
> But, when we try to kexec boot something other than the
> Linux kernel, the cleanup cannot be relied upon.
> Hence, cleanup before we go down for reboot.
>
> Keeping the cleanup at initialization also, in case BIOS
> leaves the IOMMU enabled.
>
> I considered turning off iommu only during kexec reboot, but a clean
> shutdown seems always a good idea. But if someone wants to make it
> conditional, such as VMM live update, we can do that. There doesn't
> seem to be such a condition at this time.
>
> Tested that before, the info message
> 'DMAR: Translation was enabled for <iommu> but we are not in kdump mode'
> would be reported for each iommu. The message will not appear when the
> DMA-remapping is not enabled on entry to the kernel.
>
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> ---
> Changes since v1:
> * move shutdown registration to iommu detection
>
> drivers/iommu/dmar.c | 5 ++++-
> drivers/iommu/intel-iommu.c | 20 ++++++++++++++++++++
> include/linux/dmar.h | 2 ++
> 3 files changed, 26 insertions(+), 1 deletion(-)
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread