On Mon, Nov 15, 2021 at 03:31:39PM +0100, Jan Beulich wrote:
> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
> when ACPI tables are missing") deals with apic_x2apic_probe() as called
> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
> affected: The call needs to occur after acpi_iommu_init(), such that
> iommu_intremap getting disabled there can be properly taken into account
> by apic_x2apic_probe().
>
> Note that, for the time being (further cleanup patches following),
> reversing the order of the calls to generic_apic_probe() and
> acpi_boot_init() is not an option:
> - acpi_process_madt() calls clustered_apic_check() and hence relies on
> genapic to have got filled before,
> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
> occur after generic_apic_probe(),
> - acpi_parse_madt() (called from acpi_process_madt()) calls
> acpi_madt_oem_check(), which wants to be after generic_apic_probe().
>
> Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>
Just one comment below.
> ---
> Based on code inspection only - I have no affected system and hence no
> way to actually test the case.
> ---
> v2.2: Move generic_apic_probe() again, but only past acpi_iommu_init().
> v2.1: Respect acpi_disabled in acpi_iommu_init().
> v2: Don't move generic_apic_probe() invocation, instead pull out
> acpi_iommu_init() from acpi_boot_init().
> ---
> 4.16: While investigating the issue addressed by the referenced commit,
> a variant of that problem was identified when firmware pre-enables
> x2APIC mode. Whether that's something sane firmware would do when
> at the same time IOMMU(s) is/are disabled is unclear, so this may
> be a purely academical consideration. Working around the problem
> also ought to be as simple as passing "iommu=no-intremap" on the
> command line. Considering the fragility of the code (as further
> demonstrated by v1 having been completely wrong), it may therefore
> be advisable to defer this change until after branching.
> Nevertheless it will then be a backporting candidate, so
> considering to take it right away can't simply be put off.
>
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
>
> acpi_mmcfg_init();
>
> - acpi_iommu_init();
> -
> erst_init();
>
> acpi_hest_init();
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1700,15 +1700,30 @@ void __init noreturn __start_xen(unsigne
>
> dmi_scan_machine();
>
> - generic_apic_probe();
> -
> mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
> RANGESETF_prettyprint_hex);
>
> xsm_multiboot_init(module_map, mbi);
>
> + /*
> + * IOMMU-related ACPI table parsing may require some of the system
> domains
> + * to be usable.
I would be a bit more specific and likely add "...to be usable in
order to hide IOMMU PCI devices.".
Thanks, Roger.
