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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.10.2019 17:19, Roger Pau Monné  wrote:
> On Thu, Oct 10, 2019 at 03:46:45PM +0200, Jan Beulich wrote:
>> On 10.10.2019 15:12, Roger Pau Monné  wrote:
>>> On Thu, Oct 10, 2019 at 02:55:02PM +0200, Jan Beulich wrote:
>>>> 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?
>>>
>>> I don't think the question is who enables interrupt remapping in the
>>> resume case (which is resume_x2apic when calling iommu_enable_x2apic
>>> AFAICT), the point here is that on resume the entries in the IO-APIC
>>> will already match the state of interrupt remapping, so they shouldn't
>>> be translated. If interrupt remapping was off before suspend it will
>>> still be off after resume, and there won't be any translation needed.
>>> The same is true if interrupt remapping is on before suspend.
>>
>> I disagree: save_IO_APIC_setup() gets called from resume_x2apic(),
>> not prior to suspend.
> 
> Oh, so maybe that's a misunderstanding on my side. I don't seem to be
> able to find a statement about the contents of the IO-APIC registers
> (and more specifically the entries) when getting back from
> suspension. Are all entries cleared and masked?
> 
> Are the values previous to suspension stored?

See ioapic_suspend() / ioapic_resume(): Looks like there's some
redundancy here - I don't think it makes sense for the LAPIC
code to fiddle with all the RTEs if subsequently (I assume;
didn't check) they'll all be overwritten anyway. I would seem
more logical to me if they'd just all get masked for IOMMU
enabling, deferring to ioapic_resume() for everything else.

>>>>>> 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.
>>>
>>> Right, and that's why the call to restore_IO_APIC_setup in
>>> x2apic_bsp_setup uses !x2apic_enabled as it's second parameter. If
>>> interrupt remapping has been enabled by the call to
>>> iommu_enable_x2apic x2apic_enabled must be true, and hence the entries
>>> in the IO-APIC need to be translated to use the interrupt remapping
>>> table. There's no path that can lead to restore_IO_APIC_setup with
>>> interrupt remapping enabled and x2APIC mode disabled (or with x2APIC
>>> enabled and interrupt remapping disabled).
>>>
>>> Hence if interrupt remapping is off before calling x2apic_bsp_setup
>>> (which is what Xen expects to function properly) and x2apic_enabled ==
>>> true when calling restore_IO_APIC_setup it means interrupt remapping
>>> got enabled, and the IO-APIC entries need translation.
>>
>> But the code in question sits on a shared success/error path, and
>> in the error case it matters whether x2apic_enabled was true already
>> on entry.
> 
> But it's simply not possible to reach the call to
> restore_IO_APIC_setup with x2apic_enabled == true and interrupt
> remapping disabled, regardless of the initial value of
> x2apic_enabled.
> 
> All the paths that could lead to this scenario are short-circuited
> above with a panic.

Hmm, true. Nevertheless it would feel better if the conditionals
were using what actually matters, rather than something derived.

>> I realize that io_apic_write() would suitably avoid going
>> the remapping path, but I think it would be more clear if the
>> distinction was already made properly at the call site.
> 
> I'm afraid I'm slightly loss, do you mean to replace the
> ioapic_write_entry with an io_apic_write in restore_IO_APIC_setup?

No, because of ...

> That would be the same as always passing raw == false AFAICT.

... this. I'm asking to pass in the argument for "raw" based
on what you want, without relying on io_apic_write()'s
behavior. That's more for code clarity than actual correctness,
since - as said - io_apic_write() would invoke __io_apic_write()
anyway. By setting "raw" correctly you can simply avoid going
through io_apic_write() altogether.

Jan

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