While commit XXXXXXXXXXXX ("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_boot_init() (which is what
calls acpi_iommu_init()), such that iommu_intremap getting disabled
there can be properly taken into account by apic_x2apic_probe().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Based on code inspection only - I have no affected system and hence no
way to actually test the case.

--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1694,8 +1694,6 @@ 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);
 
@@ -1705,6 +1703,13 @@ void __init noreturn __start_xen(unsigne
 
     acpi_boot_init();
 
+    /*
+     * Requires initial ACPI table parsing to have happened, such that
+     * check_x2apic_preenabled() would be able to observe acpi_iommu_init()'s
+     * findings, in particular it turning off iommu_intremap.
+     */
+    generic_apic_probe();
+
     if ( smp_found_config )
         get_smp_config();
 

On Wed, Nov 03, 2021 at 03:40:55PM +0100, Jan Beulich wrote:
> While commit XXXXXXXXXXXX ("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_boot_init() (which is what
> calls acpi_iommu_init()), such that iommu_intremap getting disabled
> there can be properly taken into account by apic_x2apic_probe().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

LGTM. I cannot find any dependency between acpi_boot_init and having
initialized the apic.

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

Thanks, Roger.

On 04.11.2021 15:21, Roger Pau Monné wrote:
> On Wed, Nov 03, 2021 at 03:40:55PM +0100, Jan Beulich wrote:
>> While commit XXXXXXXXXXXX ("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_boot_init() (which is what
>> calls acpi_iommu_init()), such that iommu_intremap getting disabled
>> there can be properly taken into account by apic_x2apic_probe().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> LGTM. I cannot find any dependency between acpi_boot_init and having
> initialized the apic.

Sadly there is, and I've now learned that when believing I would test
the change yesterday I actually didn't (or else I would have spotted
the problem that I've spotted now), and instead I did boot an older
binary: acpi_process_madt() calls clustered_apic_check() and hence
relies on genapic to have got filled before.

I'll have to see if I can come up with some variant avoiding the issue,
but I suspect that's not going to be 4.16 material anymore then. My
first try is likely going to be to simply pull out acpi_iommu_init()
from acpi_boot_init().

Jan

Jan Beulich writes ("[PATCH][4.16] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
> While commit XXXXXXXXXXXX ("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_boot_init() (which is what
> calls acpi_iommu_init()), such that iommu_intremap getting disabled
> there can be properly taken into account by apic_x2apic_probe().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Based on code inspection only - I have no affected system and hence no
> way to actually test the case.

Do we have any tests for this ?  I see it's tagged for 4.16 (and I'm
favourably inclined) but I'm not sure I follow the implications.

Roger Pau Monné writes ("Re: [PATCH][4.16] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
> LGTM. I cannot find any dependency between acpi_boot_init and having
> initialized the apic.
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks,
Ian.

On 04.11.2021 15:59, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH][4.16] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
>> While commit XXXXXXXXXXXX ("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_boot_init() (which is what
>> calls acpi_iommu_init()), such that iommu_intremap getting disabled
>> there can be properly taken into account by apic_x2apic_probe().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Based on code inspection only - I have no affected system and hence no
>> way to actually test the case.
> 
> Do we have any tests for this ?

If you mean in osstest, then I'm unaware of any, but I also don't have
a clear view on how much x2APIC-capable hardware we have, and whether
among those there are any where the firmware pre-enables x2APIC.

>  I see it's tagged for 4.16 (and I'm
> favourably inclined) but I'm not sure I follow the implications.

The main aspect here is: This is the other side of the medal as to the
referenced earlier change (which I did commit an hour or so ago).

Jan