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 Tue, Nov 02, 2021 at 11:07:25AM +0100, Jan Beulich wrote:
> On 22.10.2021 17:52, Roger Pau Monné wrote:
> > 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>
> 
> Thanks.
> 
> >> ---
> >> 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.
> 
> So what's your thought then: Make the change (in another patch), or rather
> leave the code as is? I'm slightly in favor of making the change seeing
> that you agree that the rearrangement looks to be correct.

Sorry this wasn't clear, I've rambled too much: making the change in
another patch would be my preferred option.

I still wonder whether we could place some kind of checks to ensure
the logic of interrupt remap vs APIC initialization is executed in the
right order, but that's not release material.

Thanks, Roger.