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

[David Gibson]

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

signature.asc
Description: PGP signature