Hi Yi, David, On 7/24/19 6:57 AM, Liu, Yi L wrote: >> From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf >> Of David Gibson >> Sent: Tuesday, July 23, 2019 11:58 AM >> To: Liu, Yi L <yi.l....@intel.com> >> 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: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] >>>> On Behalf Of David Gibson >>>> Sent: Wednesday, July 17, 2019 11:07 AM >>>> To: Liu, Yi L <yi.l....@intel.com> >>>> 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: kvm-ow...@vger.kernel.org >>>>>> [mailto:kvm-ow...@vger.kernel.org] On >>>> Behalf >>>>>> Of David Gibson >>>>>> Sent: Monday, July 15, 2019 10:55 AM >>>>>> To: Liu, Yi L <yi.l....@intel.com> >>>>>> 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. > So if I understand we now head towards introducing new notifiers taking a "SVMContext" as argument instead of an IOMMUMemoryRegion.
I think we need to be clear about how both abstractions (SVMContext and IOMMUMemoryRegion) differ. I would also need "SVMContext" abstraction for 2stage SMMU integration (to notify stage 1 config changes and MSI bindings) so I would need this new object to be not too much tied to SVM use case. Thanks Eric
