Re: [PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

On 9/9/2024 10:16 PM, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 01:38:18PM -0300, Jason Gunthorpe wrote:
>> On Fri, Sep 06, 2024 at 12:13:04PM +0000, Suravee Suthikulpanit wrote:
>>> According to the AMD IOMMU spec, the IOMMU reads the entire DTE either
>>> in two 128-bit transactions or a single 256-bit transaction.
>>
>> .. if two 128-bit transaction on the read side is possible then you
>> need flushing! :(
>>
>> For instance this:
>>
>>    IOMMU         CPU
>> Read [0]
>>                Write [0]
>>                Write [1]
>> Read [1]
>>
>> Will result in the iommu seeing torn incorrect data - the Guest paging
>> mode may not match the page table pointer, or the VIOMMU data may
>> become mismatched to the host translation.
>>
>> Avoiding flushing is only possible if the full 256 bits are read
>> atomically.
> 
> Also, please think about what qemu does when paravirtualizing
> this. qemu will read the DTE entry using the CPU.
> 
> For your above remark it should be reading using two 128 bit loads.
> 
> However, it doesn't seem to be doing that:
> 
> static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry)
> {
>      uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE;
> 
>      if (dma_memory_read(&address_space_memory, s->devtab + offset, entry,
>                          AMDVI_DEVTAB_ENTRY_SIZE, MEMTXATTRS_UNSPECIFIED)) {
> 
> 
> The dma_memory_read eventually boils down to memcpy()
> 
> So qemu looks wrong to me.
> 
> Jason

Thanks for pointing out. Let me check QEMU and see how to update the code.

Thanks,
Suravee