Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote:
> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
> mode (physical vs clustered) depends on iommu_intremap, that variable
> needs to be set to off as soon as we know we can't / won't enable
> interrupt remapping, i.e. in particular when parsing of the respective
> ACPI tables failed. Move the turning off of iommu_intremap from AMD
> specific code into acpi_iommu_init(), accompanying it by clearing of
> iommu_enable.
> 
> Take the opportunity and also fully skip ACPI table parsing logic on
> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway,
> like was already the case for AMD.
> 
> The tag below only references the commit uncovering a pre-existing
> anomaly.
> 
> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> While the change here deals with apic_x2apic_probe() as called from
> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
> similarly affected. That call occurs before acpi_boot_init(), which is
> what calls acpi_iommu_init(). The ordering in setup.c is in part
> relatively fragile, which is why for the moment I'm still hesitant to
> move the generic_apic_probe() call down. Plus I don't have easy access
> to a suitable system to test this case. Thoughts?

Indeed, that seems it could go quite wrong, as apic_x2apic_probe will
see iommu_intremap == iommu_intremap_full (the default value) and thus
could choose cluster mode without real interrupt remapping support.

At first sight it would seem possible to move lower down, but as you
say, this is all quite fragile. It's even made worse because we lack a
strict ordering discipline or any kind of dependency checking, so even
if we mess up the order it's likely to go unnoticed unless someone
tests on an affected system.

While we can try to solve this for the upcoming release, long term we
need a stricter ordering, either as a comment, or even better enforced
somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple
times and we should see about implementing a more robust solution.

Thanks, Roger.