Re: [Xen-devel] [PATCH v2 1/2] x2APIC: translate IO-APIC entries when enabling the IOMMU

[Jan Beulich <jbeulich@suse.com>]

On 10.10.2019 13:33, Roger Pau Monne wrote:
> When interrupt remapping is enabled as part of enabling x2APIC the

Perhaps "unmasked" instead of "the"?

> IO-APIC entries also need to be translated to the new format and added
> to the interrupt remapping table.
> 
> This prevents IOMMU interrupt remapping faults when booting on
> hardware that has unmasked IO-APIC pins.

But in the end it only papers over whatever the spurious interrupts
result form, doesn't it? Which isn't to say this isn't an
improvement. Calling out the ExtInt case here may be worthwhile as
well, as would be pointing out that this case still won't work on
AMD IOMMUs.

> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -515,7 +515,7 @@ static void resume_x2apic(void)
>      iommu_enable_x2apic();
>      __enable_x2apic();
>  
> -    restore_IO_APIC_setup(ioapic_entries);
> +    restore_IO_APIC_setup(ioapic_entries, true);
>      unmask_8259A();
>  
>  out:
> @@ -961,7 +961,12 @@ void __init x2apic_bsp_setup(void)
>          printk("Switched to APIC driver %s\n", genapic.name);
>  
>  restore_out:
> -    restore_IO_APIC_setup(ioapic_entries);
> +    /*
> +     * NB: do not use raw mode when restoring entries if the iommu has been
> +     * enabled during the process, because the entries need to be translated
> +     * and added to the remapping table in that case.
> +     */
> +    restore_IO_APIC_setup(ioapic_entries, !x2apic_enabled);

How is this different in the resume_x2apic() case? The IOMMU gets
enabled in the course of that as well. I.e. I'd expect you want
to pass "false" there, not "true".

Also how would "x2apic_enabled" reflect the transition? It may
have been "true" already before entering the function here.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel