On 19.07.2019 19:27, Andrew Cooper wrote:
> On 16/07/2019 17:38, Jan Beulich wrote:
>> This is in preparation of actually enabling x2APIC mode, which requires
>> this wider IRTE format to be used.
>>
>> A specific remark regarding the first hunk changing
>> amd_iommu_ioapic_update_ire(): This bypass was introduced for XSA-36,
>> i.e. by 94d4a1119d ("AMD,IOMMU: Clean up old entries in remapping
>> tables when creating new one"). Other code introduced by that change has
>> meanwhile disappeared or further changed, and I wonder if - rather than
>> adding an x2apic_enabled check to the conditional - the bypass couldn't
>> be deleted altogether. For now the goal is to affect the non-x2APIC
>> paths as little as possible.
>
> There are plenty of mistakes with XSA-36. Reading the XSA back, the
> MITIGATION section gets the sense of the iommu=amd-iommu-perdev-intremap
> boolean the wrong way around. Oh well...
>
> SP5100 erratum 28 only requires that the IDE and SATA devices share
> tables, not that every device on the whole system shares tables.
>
> With the proposed work to perform IOMMU assignment by group rather than
> individually, this will naturally fall out as a quirk requiring the two
> devices to be grouped, at which point we can drop all remnants of global
> remapping tables.
Yes, and I'll be happy to see them go away.
> In this case, I'm not sure it is worth caring about shared-table mode on
> x2apic-capable systems. 0 people will be using that mode. That said,
> if its easier to wait until the IOMMU changes to make this adjustment,
> then fine.
It certainly is, especially with backporting of this series in mind.
>> @@ -142,7 +178,15 @@ static void free_intremap_entry(const st
>> {
>> union irte_ptr entry = get_intremap_entry(iommu, bdf, index);
>>
>> - ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
>> + if ( iommu->ctrl.ga_en )
>> + {
>> + ACCESS_ONCE(entry.ptr128->raw[0]) = 0;
>> + /* Low half (containing RemapEn) needs to be cleared first. */
>> + barrier();
>
> While this will function on x86, I still consider this buggy. From a
> conceptual point of view, barrier() is not the correct construction to
> use, whereas smp_wmb() is.
I think it's the 3rd time now that I respond saying that barrier() is
as good or as bad as smp_wmb(), just for different reasons. While I
agree with you that barrier() is correct on x86 only, I'm yet to hear
back from you on my argument that smp_wmb() is incorrect when
considering its UP semantics (which we don't currently implement, but
which Linux as the origin of the construct can well be used for
reference). And I think we both don't really want wmb() here.
> As this is the only remaining issue, with it fixed in each location,
> Acked-by: Andrew Cooper <andrew.coop...@citrix.com>
I'm not going to apply this for now, until we've managed to come to an
agreement on the item above.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
