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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.10.2019 14:13, Roger Pau Monné  wrote:
> On Thu, Oct 10, 2019 at 01:54:06PM +0200, Jan Beulich wrote:
>> 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.
> 
> But the fix for the ExtINT AMD issue should be done in
> amd_iommu_ioapic_update_ire then, so that it can properly handle
> ExtINT delivery mode, not to this part of the code. I will look
> into it, but I think it's kind of tangential to the issue here.

I'm not talking of you working on fixing this right away. I'm merely
asking that you mention here (a) the ExtInt special case and (b)
that this special case will (continue to) not work in the AMD case.

>>> --- 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".
> 
> In the resume_x2apic case interrupt remapping should already be
> enabled or not, but that function is not going to enable interrupt
> remapping if it wasn't enabled before, hence the IO-APIC entries
> should already be using the interrupt remapping table and no
> translation is needed.

Who / what would have enabled the IOMMU in the resume case?

>> Also how would "x2apic_enabled" reflect the transition? It may
>> have been "true" already before entering the function here.
> 
> If x2apic_enabled == true at the point where restore_IO_APIC_setup
> is called interrupt remapping must be enabled, because AFAICT at this
> point it's not possible to have x2apic_enabled == true and interrupt
> remapping disabled.
> 
> The issue I can see here is what happens if interrupt remapping was
> already enabled by the hardware, and entries in the IO-APIC are
> already using the remapping table. I would have to look into how to
> detect that case properly, but I think the proposed change is an
> improvement overall.

Firmware handing off with the IOMMU (and hence interrupt remapping)
already enabled is a case which - afaict - we can't currently cope
with. Firmware handing off in x2APIC enabled state is typically
with the IOMMU (and hence interrupt remapping) still disabled. This
is not a forbidden mode, it's just that in such a configuration
interrupts can't be delivered to certain CPUs.

In any event you need to properly distinguish x2APIC enabled state
(or the transition thereof) from IOMMU / interrupt remapping
enabled state (or the transition thereof). I.e. you want to avoid
raw mode restore if interrupt remapping state transitioned from
off to on in the process.

Jan

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