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

[Liu, Yi L]

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:address@hidden]
> Sent: Wednesday, July 24, 2019 5:33 PM
> To: Liu, Yi L <address@hidden>; David Gibson <address@hidden>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> 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.

yes, this is the rough idea.
 
> 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.

I agree. SVMContext is just a proposed name. We may have better naming for it
as long as the thing we want to have is a new abstract layer between VFIO and
vIOMMU. Per my understanding, the IOMMUMemoryRegion abstraction is for
the notifications around guest memory changes. e.g. VFIO needs to be notified
when there is MAP/UNMAP happened. However, for the SVMContext, it aims to
be an abstraction for SVM/PASID related operations, which has no direct
relationship with memory. e.g. for VT-d, pasid allocation, pasid bind/unbind,
pasid based-iotlb flush. I think pasid bind/unbind and pasid based-iotlb flush 
is
equivalent with the stage 1 config changes in SMMU. If you agree to use it
all the same, how about naming it as IOMMUConext? Also, pls feel free to
propose your suggestion. :-)

Thanks,
Yi Liu

changes.

> Thanks
> 
> Eric