On Wed, Jul 24, 2019 at 11:33:06AM +0200, Auger Eric wrote: > Hi Yi, David, > > On 7/24/19 6:57 AM, Liu, Yi L wrote: > >> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On Behalf > >> Of David Gibson > >> Sent: Tuesday, July 23, 2019 11:58 AM > >> To: Liu, Yi L <yi.l.liu@xxxxxxxxx> > >> 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-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] > >>>> On Behalf Of David Gibson > >>>> Sent: Wednesday, July 17, 2019 11:07 AM > >>>> To: Liu, Yi L <yi.l.liu@xxxxxxxxx> > >>>> 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-owner@xxxxxxxxxxxxxxx > >>>>>> [mailto:kvm-owner@xxxxxxxxxxxxxxx] On > >>>> Behalf > >>>>>> Of David Gibson > >>>>>> Sent: Monday, July 15, 2019 10:55 AM > >>>>>> To: Liu, Yi L <yi.l.liu@xxxxxxxxx> > >>>>>> 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. That's my suggestion. I don't really have any authority to decide.. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
