On 22.07.2019 15:36, Andrew Cooper wrote:
> On 22/07/2019 09:34, Jan Beulich wrote:
>> On 19.07.2019 19:27, Andrew Cooper wrote:
>>> On 16/07/2019 17:38, Jan Beulich wrote:
>>>> @@ -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.
>
> barrier() and smp_wmb() are different constructs, with specific,
> *different* meanings. From a programmers point of view, they should be
> considered black boxes of functionality.
>
> barrier() is for forcing the compiler to not reorder things.
>
> smp_wmb() is about the external visibility of writes, as observed by a
> different entity on a coherent fabric.
I'm afraid I disagree here: The "smp" in its name means "CPU", not
"entity" in your sentence. Which is why ...
> The fact they alias on x86 in an implementation detail of x86 cache
> coherency - it does not mean they can legitimately be alternated in code.
>
> This piece of code is a 2-way communication between the CPU core and the
> IOMMU, over a coherent cache. The IOMMU logically has an smp_rmb() in
> its mirror functionality (although that is likely not how the property
> is expressed).
>
>> 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).
>
> UP vs SMP doesn't affect which is the correct construct to use.
... I disagree with this part too. Even nowadays Linux still has
#ifdef CONFIG_SMP
[...]
#else /* !CONFIG_SMP */
#ifndef smp_mb
#define smp_mb() barrier()
#endif
#ifndef smp_rmb
#define smp_rmb() barrier()
#endif
#ifndef smp_wmb
#define smp_wmb() barrier()
#endif
in asm-generic/barrier.h, i.e. independent of architecture. Yet the
SMP config setting is concerned about CPUs only, not "entities".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
