Re: [Qemu-devel] [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation

[Liu, Yi L]

> From: address@hidden [mailto:address@hidden] On Behalf
> Of David Gibson
> Sent: Tuesday, July 23, 2019 11:58 AM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
> > > From: address@hidden [mailto:address@hidden]
> > > On Behalf Of David Gibson
> > > Sent: Wednesday, July 17, 2019 11:07 AM
> > > To: Liu, Yi L <address@hidden>
> > > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> > > implementation
> > >
> > > On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> > > > > From: address@hidden
> > > > > [mailto:address@hidden] On
> > > Behalf
> > > > > Of David Gibson
> > > > > Sent: Monday, July 15, 2019 10:55 AM
> > > > > To: Liu, Yi L <address@hidden>
> > > > > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> > > > > implementation
> > > > >
> > > > > On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > > > > > This patch adds vfio implementation 
> > > > > > PCIPASIDOps.alloc_pasid/free_pasid().
> > > > > > These two functions are used to propagate guest pasid
> > > > > > allocation and free requests to host via vfio container ioctl.
> > > > >
> > > > > As I said in an earlier comment, I think doing this on the
> > > > > device is conceptually incorrect.  I think we need an explcit
> > > > > notion of an SVM context (i.e. the namespace in which all the
> > > > > PASIDs live) - which will IIUC usually be shared amongst
> > > > > multiple devices.  The create and free PASID requests should be on 
> > > > > that object.
> > > >
> > > > Actually, the allocation is not doing on this device. System wide,
> > > > it is done on a container. So not sure if it is the API interface
> > > > gives you a sense that this is done on device.
> > >
> > > Sorry, I should have been clearer.  I can see that at the VFIO level
> > > it is done on the container.  However the function here takes a bus
> > > and devfn, so this qemu internal interface is per-device, which
> > > doesn't really make sense.
> >
> > Got it. The reason here is to pass the bus and devfn info, so that
> > VFIO can figure out a container for the operation. So far in QEMU,
> > there is no good way to connect the vIOMMU emulator and VFIO regards
> > to SVM.
> 
> Right, and I think that's an indication that we're not modelling something in 
> qemu
> that we should be.
> 
> > hw/pci layer is a choice based on some previous discussion. But yes, I
> > agree with you that we may need to have an explicit notion for SVM. Do
> > you think it is good to introduce a new abstract layer for SVM (may
> > name as SVMContext).
> 
> I think so, yes.
> 
> If nothing else, I expect we'll need this concept if we ever want to be able 
> to
> implement SVM for emulated devices (which could be useful for debugging, even 
> if
> it's not something you'd do in production).
> 
> > The idea would be that vIOMMU maintain the SVMContext instances and
> > expose explicit interface for VFIO to get it. Then VFIO register
> > notifiers on to the SVMContext. When vIOMMU emulator wants to do PASID
> > alloc/free, it fires the corresponding notifier. After call into VFIO,
> > the notifier function itself figure out the container it is bound. In
> > this way, it's the duty of vIOMMU emulator to figure out a proper
> > notifier to fire. From interface point of view, it is no longer
> > per-device.
> 
> Exactly.

Cool, let me prepare another version with the ideas. Thanks for your
review. :-)

Regards,
Yi Liu

> > Also, it leaves the PASID management details to vIOMMU emulator as it
> > can be vendor specific. Does it make sense?
> > Also, I'd like to know if you have any other idea on it. That would
> > surely be helpful. :-)
> >
> > > > Also, curious on the SVM context
> > > > concept, do you mean it a per-VM context or a per-SVM usage context?
> > > > May you elaborate a little more. :-)
> > >
> > > Sorry, I'm struggling to find a good term for this.  By "context" I
> > > mean a namespace containing a bunch of PASID address spaces, those
> > > PASIDs are then visible to some group of devices.
> >
> > I see. May be the SVMContext instance above can include multiple PASID
> > address spaces. And again, I think this relationship should be
> > maintained in vIOMMU emulator.