Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Roger Pau Monné <royger@xxxxxxxxxxx>]

On Fri, Nov 05, 2021 at 01:32:18PM +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 <jbeulich@xxxxxxxx>
> ---
> Based on code inspection only - I have no affected system and hence no
> way to actually test the case.
> ---
> 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.

The main issue here would be missing a dependency between
acpi_iommu_init and the rest of acpi_boot_init, apart from the
existing dependencies between acpi_iommu_init and generic_apic_probe.

> 
> --- 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
> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>  
>      dmi_scan_machine();
>  
> +    /*
> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, 
> for
> +     * check_x2apic_preenabled() to be able to observe respective findings, 
> in
> +     * particular iommu_intremap having got turned off.
> +     */
> +    acpi_iommu_init();

If we pull this out I think we should add a check for acpi_disabled
and if set turn off iommu_intremap and iommu_enable?

Thanks, Roger.