Re: [Xen-devel] [PATCH v2 05/10] AMD/IOMMU: introduce 128-bit IRTE non-guest-APIC IRTE format

[Jan Beulich <JBeulich@xxxxxxxx>]

On 02.07.2019 16:41, Andrew Cooper wrote:
> On 27/06/2019 16:21, Jan Beulich wrote:
>> @@ -142,7 +187,21 @@ static void free_intremap_entry(unsigned
>>   {
>>       union irte_ptr entry = get_intremap_entry(seg, bdf, index);
>>   
>> -    ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
>> +    switch ( irte_mode )
>> +    {
>> +    case irte32:
>> +        ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
>> +        break;
>> +
>> +    case irte128:
>> +        ACCESS_ONCE(entry.ptr128->raw[0]) = 0;
>> +        barrier();
> 
> smp_wmb().
> 
> Using barrier here isn't technically correct, because what matters is
> the external visibility of the write.
> 
> It functions correctly on x86 because smp_wmb() is barrier(), but this
> code doesn't work correctly on e.g. ARM.

Well, I did reply to a similar earlier comment of yours, and I
had hoped to get a reply from you in turn before actually sending
out v2. As said there, smp_wmb() isn't correct either, yet you
also don't want wmb() here. Even if we don't patch them ourselves,
we should still follow the abstract Linux model and _assume_
smp_*mb() convert to no-op when running on a UP system. The
barrier, however, is needed even in that case.

What I'm okay to do is accompany the barrier() (or, if you insist,
smp_wmb()) use with a comment clarifying that this is fine for x86,
but would need changing if the code was included in builds for
other architectures.

>> @@ -444,9 +601,9 @@ static int update_intremap_entry_from_ms
>>       unsigned long flags;
>>       union irte_ptr entry;
>>       u16 req_id, alias_id;
>> -    u8 delivery_mode, dest, vector, dest_mode;
>> +    uint8_t delivery_mode, vector, dest_mode;
> 
> For the ioapic version, you used unsigned int, rather than uint8_t.  I'd
> expect them to at least be consistent.

The type change on the I/O-APIC side is because "dest" is among
the variables there. But looking at both changes again, I guess
I'll rather use the approach here also in the I/O-APIC function,
moving "dest" down together with "offset".

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