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

[Suthikulpanit, Suravee]

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