Re: [Xen-devel] [PATCH v3 08/14] AMD/IOMMU: introduce 128-bit IRTE non-guest-APIC IRTE format

[Jan Beulich <JBeulich@xxxxxxxx>]

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.cooper3@xxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel