Date:   Fri, 30 Oct 2020 11:18:27 +0100
From:   Jean-Philippe Brucker <jean-philippe@...aro.org>
To:     Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:     Jacob Pan <jacob.pan.linux@...il.com>,
        iommu@...ts.linux-foundation.org,
        LKML <linux-kernel@...r.kernel.org>,
        Joerg Roedel <joro@...tes.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Jonathan Corbet <corbet@....net>, linux-api@...r.kernel.org,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        Eric Auger <eric.auger@...hat.com>,
        Yi Liu <yi.l.liu@...el.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        Raj Ashok <ashok.raj@...el.com>, Wu Hao <hao.wu@...el.com>,
        Yi Sun <yi.y.sun@...el.com>, Dave Jiang <dave.jiang@...el.com>,
        Randy Dunlap <rdunlap@...radead.org>, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 01/14] docs: Document IO Address Space ID (IOASID) APIs

On Mon, Oct 26, 2020 at 02:05:06PM -0700, Jacob Pan wrote:
> > This looks good to me, with small comments below.
> > 
> Can I add your Reviewed-by tag after addressing the comments?

Yes sure, this took forever to review so I'm happy not to do another pass :)


> > > +Each IOASID set is created with a token, which can be one of the
> > > +following token types:
> > > +
> > > + - IOASID_SET_TYPE_NULL (Arbitrary u64 value)  
> > 
> > Maybe NULL isn't the best name then. NONE?
> > 
> Agreed, 'NONE' makes more sense.

Although patch 5 only allows a NULL token for this type. So the name seems
fine, you could just fix this description.


> > > +IOASID core has the notion of "custom allocator" such that guest can
> > > +register virtual command allocator that precedes the default one.  
> > 
> > "Supersedes", rather than "precedes"?
> > 
> My understanding is that 'supersede' means replace something but 'precede'
> means get in front of something. I do want to emphasis that the custom
> allocator takes precedence over the default allocator.

Right it's ambiguous. The custom allocator does entirely replace the
allocation action, but the default one is still used for storage. Anyway,
you can leave this.


> > > +Let's examine the IOASID life cycle again when free happens *before*
> > > +unbind. This could be a result of misbehaving guests or crash. Assuming
> > > +VFIO cannot enforce unbind->free order. Notice that the setup part up
> > > +until step #12 is identical to the normal case, the flow below starts
> > > +with step 13.
> > > +
> > > +::
> > > +
> > > +     VFIO        IOMMU        KVM        VDCM        IOASID       Ref
> > > +   ..................................................................
> > > +   13 -------- GUEST STARTS DMA --------------------------
> > > +   14 -------- *GUEST MISBEHAVES!!!* ----------------
> > > +   15 ioasid_free()
> > > +   16                                             ioasid_notify(FREE)
> > > +   17                                             mark_free_pending
> > > (1)  
> > 
> > Could we use superscript ¹²³⁴ for footnotes? These look like function
> > parameters
> > 
> yes, much better
> 
> > > +   18                          kvm_nb_handler(FREE)
> > > +   19                          vmcs_update_atomic()
> > > +   20                          ioasid_put_locked()   ->           3
> > > +   21                                   vdcm_nb_handler(FREE)
> > > +   22            iomm_nb_handler(FREE)  
> > 
> > iommu_nb_handler
> > 
> got it
> 
> > > +   23 ioasid_free() returns(2)          schedule_work()           2  
> > 
> > I completely lost track here, couldn't figure out in which direction to
> > read the diagram. What work is scheduled?
> The time line goes downward but we only control the notification order in
> terms of when the events are received. Some completions are async thus out
> of order done by work items. The only in-order completion is the KVM update
> of its PASID translation table.
> 
> After #23, the async works are scheduled to complete clean up work outside
> the spinlock(held by the caller of the atomic notifier).
> 
> Any suggestions to improve the readability of the time line?

Maybe explain what happens from line 23: ioasid_free() schedules... a FREE
notification? Which happens on line 24 (corresponding to the second
schedule_work()?) and is handled by (a) VDCM to clear the device context
and (b) IOMMU to clear the PASID context, both ending up dropping their
ref.

> 
> > Why does the IOMMU driver drop
> > its reference to the IOASID before unbdind_gpasid()?
> > 
> This is the exception case where userspace issues IOASID free before
> unbind_gpasid(). The equivalent of unbind is performed in the IOASID_FREE
> notification handler. In IOASID_FREE handler, reference is dropped and
> private data deleted. After that, if unbind comes to IOMMU driver, it will
> not find IOASID private data therefore just return.

Right ok. As you noted below the damage is caused by and limited to the
guest, so I think it's fine.

> 
> > > +   24            schedule_work()        vdev_clear_wk(hpasid)
> > > +   25            teardown_pasid_wk()
> > > +   26                                   ioasid_put() ->           1
> > > +   27            ioasid_put()                                     0
> > > +   28                                                 Reclaimed
> > > +   29 unbind_gpasid()
> > > +   30            iommu_unbind()->ioasid_find() Fails(3)
> > > +   -------------- New Life Cycle Begin ----------------------------
> > > +
> > > +Note:
> > > +
> > > +1. By marking IOASID FREE_PENDING at step #17, no new references can be
> > > +   held. ioasid_get/find() will return -ENOENT;  
> > 
> > s/held/taken
> > 
> Got it.
> 
> > Thanks,
> > Jean
> > 
> > > +2. After step #23, all events can go out of order. Shall not affect
> > > +   the outcome.
> > > +3. IOMMU driver fails to find private data for unbinding. If unbind is
> > > +   called after the same IOASID is allocated for the same guest again,
> > > +   this is a programming error. The damage is limited to the guest
> > > +   itself since unbind performs permission checking based on the
> > > +   IOASID set associated with the guest process.

"guest process" can be confusing (process run by the guest?), just "guest"
might be better.

Thanks,
Jean

Powered by blists - more mailing lists