Re: [Xen-devel] [PATCH 3/9] AMD/IOMMU: use bit field for IRTE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.06.19 at 13:31, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/06/2019 14:23, Jan Beulich wrote:
>> At the same time restrict its scope to just the single source file
>> actually using it, and abstract accesses by introducing a union of
>> pointers. (A union of the actual table entries is not used to make it
>> impossible to [wrongly, once the 128-bit form gets added] perform
>> pointer arithmetic / array accesses on derived types.)
>>
>> Also move away from updating the entries piecemeal: Construct a full new
>> entry, and write it out.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
>> actual writes, but both cast the value to a scalar one, which doesn't
>> suit us here (and I also didn't want to make the compound type a union
>> with a raw member just for this).
> 
> Actually, having looked at the following patch, I think it would be
> better to union with a uint32_t raw, so that we can use...

Well, I did in fact have it that way first, until ...

>> +static void update_intremap_entry(union irte_ptr entry, unsigned int vector,
>> +                                  unsigned int int_type,
>> +                                  unsigned int dest_mode, unsigned int dest)
>> +{
>> +    struct irte_basic basic = {
>> +        .remap_en = 1,
>> +        .sup_io_pf = 0,
>> +        .int_type = int_type,
>> +        .rq_eoi = 0,
>> +        .dm = dest_mode,
>> +        .dest = dest,
>> +        .vector = vector,
>> +    };

... I figured that this initializer then will require the bitfields part of
the union to also get named, like for union amd_iommu_ext_features
in patch 1.

>> +    *entry.basic = basic;
> 
> ACCESS_ONCE(*entry.basic.raw) = basic.raw.
> 
> The problem is in an unoptimised case, structure assignment in
> implemented with memcpy(), which may be implemented as `rep stosb` which
> may result in a spliced write with the enable bit set first.
> 
> Using a union with raw allows for the use of ACCESS_ONCE(), which forces
> the compiler to implement them as 32bit single mov's.

If we are worried about this, writing of 32-bit entries could be done
(as an alternative to what you suggest) just like that of 128-bit
ones by the last patch in the series.

Jan



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