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 15:29, 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.
> 
> Please bear with me, I've taken a look at the ExtINT handling for AMD
> and I'm not sure I can spot what's missing. Xen seems to handle both
> the EIntPass and IV fields of the DTE (see iommu_dte_add_device_entry
> and amd_iommu_set_intremap_table), and that should be enough in order
> to passthrough such interrupts.

EIntPass gets set based on ACPI table information, not what we found
in a particular RTE. That's hopefully fine, but you know how reliable
firmware is especially in corner cases.

> Is there some other handling that I'm missing? (maybe in the handling
> of the interrupt itself?)

Look at the update_intremap_entry_from_ioapic() ->
update_intremap_entry() path: This converts the 3-bit delivery mode
field into a 1-bit int_type one (the field is really 3 bits wide,
but values 010..111 (binary) are documented as reserved; I can't
exclude though that the documentation is wrong here).

Jan

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