Re: [PATCH v3 01/14] docs: Document IO Address Space ID (IOASID) APIs

[Jacob Pan]

Hi Jean-Philippe,

On Fri, 30 Oct 2020 11:18:27 +0100, Jean-Philippe Brucker
<jean-philippe@xxxxxxxxxx> wrote:

> 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 :)
> 
I am afraid I have to ask for another round of reviews since it was
suggested to keep IOASID allocation interface independent, instead of being
part of VFIO UAPI. Yi and I are working out the details to come up with a
PoC. As you might be aware, the need for this independent interface is that
we may have multiple users of PASID, e.g VDPA, user space drivers, etc.
The IOASID user interface also has slight impact on the IOASID core code,
which is why I am slow in response to your code review. Will incorporate
your review in the next round with support of independent user API.
Much appreciated!

> 
> > > > +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.
> 
OK.

> 
> > > > +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.
> 
OK

> 
> > > > +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.
> 
Got it, I will add that.

> >   
> > > 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.
> 
OK.

> >   
> > > > +   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


Thanks,

Jacob