2024-03-07 14:07:47

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] iommu: always select INTEL_IOMMU for IRQ_REMAP

From: Arnd Bergmann <[email protected]>

CONFIG_INTR_REMAP was originally split out of the intel iommu code to be
shared by IRQ_REMAP. This recently broke again because the IRQ_REMAP
code calls the global device_rbtree_find() function that is unavailable
for builds without INTEL_IOMMU:

x86_64-linux-ld: vmlinux.o: in function `qi_submit_sync':
(.text+0x10771e0): undefined reference to `device_rbtree_find'

It seems that the intel iommu code now contains a lot of generic helper
functions that are not specific to intel, such as alloc_pgtable_page(),
iommu_flush_write_buffer(), domain_attach_iommu() etc, so presumably
it is not x86 specific any more.

Fix the build failure for now by just selectin INTEL_IOMMU by the
code that relies on it. It might be helpful to split out all the
functions without an intel_iommu_* prefix into a helper library
to avoid including the x86 specific bits on non-x86, but that could
be a follow-up.

Fixes: d3f138106b4b ("iommu: Rename the DMAR and INTR_REMAP config options")
Fixes: 80a9b50c0b9e ("iommu/vt-d: Improve ITE fault handling if target device isn't present")
Signed-off-by: Arnd Bergmann <[email protected]>
---
Not sure about this one, I just ran across the build regression and
wasn't sure if the intel-iommu functions are meant to be generic
or just misnamed. The patch description assumes the former, if that
is wrong, it needs a different explanation or a different fix.
---
drivers/iommu/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index df156f0a1a17..da5339bdb7e7 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -196,6 +196,7 @@ config IRQ_REMAP
bool "Support for Interrupt Remapping"
depends on X86_64 && X86_IO_APIC && PCI_MSI && ACPI
select DMAR_TABLE
+ select INTEL_IOMMU
help
Supports Interrupt remapping for IO-APIC and MSI devices.
To use x2apic mode in the CPU's which support x2APIC enhancements or
--
2.39.2



2024-03-07 15:48:46

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH] iommu: always select INTEL_IOMMU for IRQ_REMAP

On 07/03/2024 2:05 pm, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> CONFIG_INTR_REMAP was originally split out of the intel iommu code to be
> shared by IRQ_REMAP. This recently broke again because the IRQ_REMAP
> code calls the global device_rbtree_find() function that is unavailable
> for builds without INTEL_IOMMU:
>
> x86_64-linux-ld: vmlinux.o: in function `qi_submit_sync':
> (.text+0x10771e0): undefined reference to `device_rbtree_find'
>
> It seems that the intel iommu code now contains a lot of generic helper
> functions that are not specific to intel, such as alloc_pgtable_page(),
> iommu_flush_write_buffer(), domain_attach_iommu() etc, so presumably
> it is not x86 specific any more.

No, it's still all very much Intel-specific, which in fact means it is
just recently now x86-specific since IA-64 has departed.

Historically it's always been the case that building IRQ remapping
support on its own without IOMMU_API was supported, and for a while we
even had the awkward iommu_device_set_ops() wrapper and various other
indirections and stubs in the core API solely to make it work. IMO the
underlying issue here is that there have never been very clear lines of
separation between the ACPI DMAR code, the IOMMU API driver, and the IRQ
remapping driver, so unless the whole design could be improved to make
it harder to break, it probably is time to start asking the question of
whether anyone actually cares about this config combination any more.

Thanks,
Robin.

> Fix the build failure for now by just selectin INTEL_IOMMU by the
> code that relies on it. It might be helpful to split out all the
> functions without an intel_iommu_* prefix into a helper library
> to avoid including the x86 specific bits on non-x86, but that could
> be a follow-up.
>
> Fixes: d3f138106b4b ("iommu: Rename the DMAR and INTR_REMAP config options")
> Fixes: 80a9b50c0b9e ("iommu/vt-d: Improve ITE fault handling if target device isn't present")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> Not sure about this one, I just ran across the build regression and
> wasn't sure if the intel-iommu functions are meant to be generic
> or just misnamed. The patch description assumes the former, if that
> is wrong, it needs a different explanation or a different fix.
> ---
> drivers/iommu/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index df156f0a1a17..da5339bdb7e7 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -196,6 +196,7 @@ config IRQ_REMAP
> bool "Support for Interrupt Remapping"
> depends on X86_64 && X86_IO_APIC && PCI_MSI && ACPI
> select DMAR_TABLE
> + select INTEL_IOMMU
> help
> Supports Interrupt remapping for IO-APIC and MSI devices.
> To use x2apic mode in the CPU's which support x2APIC enhancements or

2024-03-08 01:12:03

by Baolu Lu

[permalink] [raw]
Subject: Re: [PATCH] iommu: always select INTEL_IOMMU for IRQ_REMAP

On 3/7/24 11:48 PM, Robin Murphy wrote:
> On 07/03/2024 2:05 pm, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> CONFIG_INTR_REMAP was originally split out of the intel iommu code to be
>> shared by IRQ_REMAP. This recently broke again because the IRQ_REMAP
>> code calls the global device_rbtree_find() function that is unavailable
>> for builds without INTEL_IOMMU:
>>
>> x86_64-linux-ld: vmlinux.o: in function `qi_submit_sync':
>> (.text+0x10771e0): undefined reference to `device_rbtree_find'
>>
>> It seems that the intel iommu code now contains a lot of generic helper
>> functions that are not specific to intel, such as alloc_pgtable_page(),
>> iommu_flush_write_buffer(), domain_attach_iommu() etc, so presumably
>> it is not x86 specific any more.
>
> No, it's still all very much Intel-specific, which in fact means it is
> just recently now x86-specific since IA-64 has departed.
>
> Historically it's always been the case that building IRQ remapping
> support on its own without IOMMU_API was supported, and for a while we
> even had the awkward iommu_device_set_ops() wrapper and various other
> indirections and stubs in the core API solely to make it work. IMO the
> underlying issue here is that there have never been very clear lines of
> separation between the ACPI DMAR code, the IOMMU API driver, and the IRQ
> remapping driver, so unless the whole design could be improved to make
> it harder to break, it probably is time to start asking the question of
> whether anyone actually cares about this config combination any more.

IOMMU in passthrough mode + interrupt remapping might be a reasonable
alternative to no IOMMU API + interrupt remapping.

Best regards,
baolu