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

[Jan Beulich <JBeulich@xxxxxxxx>]

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