RE: [PATCH RFC 00/19] IOMMUFD Dirty Tracking

Tian, Kevin
2022-05-06T03:14:23Z

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Thursday, May 5, 2022 7:51 PM
>
> On 5/5/22 12:03, Tian, Kevin wrote:
> >> From: Joao Martins <joao.m.martins@oracle.com>
> >> Sent: Thursday, May 5, 2022 6:07 PM
> >>
> >> On 5/5/22 08:42, Tian, Kevin wrote:
> >>>> From: Jason Gunthorpe <jgg@nvidia.com>
> >>>> Sent: Tuesday, May 3, 2022 2:53 AM
> >>>>
> >>>> On Mon, May 02, 2022 at 12:11:07PM -0600, Alex Williamson wrote:
> >>>>> On Fri, 29 Apr 2022 05:45:20 +0000
> >>>>> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >>>>>>> From: Joao Martins <joao.m.martins@oracle.com>
> >>>>>>> 3) Unmapping an IOVA range while returning its dirty bit prior to
> >>>>>>> unmap. This case is specific for non-nested vIOMMU case where an
> >>>>>>> erronous guest (or device) DMAing to an address being unmapped
> at
> >>>> the
> >>>>>>> same time.
> >>>>>>
> >>>>>> an erroneous attempt like above cannot anticipate which DMAs can
> >>>>>> succeed in that window thus the end behavior is undefined. For an
> >>>>>> undefined behavior nothing will be broken by losing some bits dirtied
> >>>>>> in the window between reading back dirty bits of the range and
> >>>>>> actually calling unmap. From guest p.o.v. all those are black-box
> >>>>>> hardware logic to serve a virtual iotlb invalidation request which just
> >>>>>> cannot be completed in one cycle.
> >>>>>>
> >>>>>> Hence in reality probably this is not required except to meet vfio
> >>>>>> compat requirement. Just in concept returning dirty bits at unmap
> >>>>>> is more accurate.
> >>>>>>
> >>>>>> I'm slightly inclined to abandon it in iommufd uAPI.
> >>>>>
> >>>>> Sorry, I'm not following why an unmap with returned dirty bitmap
> >>>>> operation is specific to a vIOMMU case, or in fact indicative of some
> >>>>> sort of erroneous, racy behavior of guest or device.
> >>>>
> >>>> It is being compared against the alternative which is to explicitly
> >>>> query dirty then do a normal unmap as two system calls and permit a
> >>>> race.
> >>>>
> >>>> The only case with any difference is if the guest is racing DMA with
> >>>> the unmap - in which case it is already indeterminate for the guest if
> >>>> the DMA will be completed or not.
> >>>>
> >>>> eg on the vIOMMU case if the guest races DMA with unmap then we
> are
> >>>> already fine with throwing away that DMA because that is how the race
> >>>> resolves during non-migration situations, so resovling it as throwing
> >>>> away the DMA during migration is OK too.
> >>>>
> >>>>> We need the flexibility to support memory hot-unplug operations
> >>>>> during migration,
> >>>>
> >>>> I would have thought that hotplug during migration would simply
> >>>> discard all the data - how does it use the dirty bitmap?
> >>>>
> >>>>> This was implemented as a single operation specifically to avoid
> >>>>> races where ongoing access may be available after retrieving a
> >>>>> snapshot of the bitmap. Thanks,
> >>>>
> >>>> The issue is the cost.
> >>>>
> >>>> On a real iommu elminating the race is expensive as we have to write
> >>>> protect the pages before query dirty, which seems to be an extra IOTLB
> >>>> flush.
> >>>>
> >>>> It is not clear if paying this cost to become atomic is actually
> >>>> something any use case needs.
> >>>>
> >>>> So, I suggest we think about a 3rd op 'write protect and clear
> >>>> dirties' that will be followed by a normal unmap - the extra op will
> >>>> have the extra oveheard and userspace can decide if it wants to pay or
> >>>> not vs the non-atomic read dirties operation. And lets have a use case
> >>>> where this must be atomic before we implement it..
> >>>
> >>> and write-protection also relies on the support of I/O page fault...
> >>>
> >> /I think/ all IOMMUs in this series already support
> permission/unrecoverable
> >> I/O page faults for a long time IIUC.
> >>
> >> The earlier suggestion was just to discard the I/O page fault after
> >> write-protection happens. fwiw, some IOMMUs also support suppressing
> >> the event notification (like AMD).
> >
> > iiuc the purpose of 'write-protection' here is to capture in-fly dirty pages
> > in the said race window until unmap and iotlb is invalidated is completed.
> >
> But then we depend on PRS being there on the device, because without it,
> DMA is
> aborted on the target on a read-only IOVA prior to the page fault, thus the
> page
> is not going to be dirty anyways.
>
> > *unrecoverable* faults are not expected to be used in a feature path
> > as occurrence of such faults may lead to severe reaction in iommu
> > drivers e.g. completely block DMA from the device causing such faults.
>
> Unless I totally misunderstood ... the later is actually what we were
> suggesting
> here /in the context of unmaping an GIOVA/(*).
>
> The wrprotect() was there to ensure we get an atomic dirty state of the IOVA
> range
> afterwards, by blocking DMA (as opposed to sort of mediating DMA). The I/O
> page fault is
> not supposed to happen unless there's rogue DMA AIUI.

You are right. It's me misunderstanding the proposal here. 😊

>
> TBH, the same could be said for normal DMA unmap as that does not make
> any sort of
> guarantees of stopping DMA until the IOTLB flush happens.
>
> (*) Although I am not saying the use-case of wrprotect() and mediating dirty
> pages you say
> isn't useful. I guess it is in a world where we want support post-copy
> migration with VFs,
> which would require some form of PRI (via the PF?) of the migratable VF. I
> was just trying
> to differentiate that this in the context of unmapping an IOVA.