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

[Auger Eric]

Hi Yi,

On 7/26/19 7:18 AM, Liu, Yi L wrote:
> 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. :-)
Sorry for the delay. Yes IOMMUContext sounds OK to me. Looking forward
to reading your next revision.

Thanks

Eric
> 
> Thanks,
> Yi Liu
> 
> changes.
> 
>> Thanks
>>
>> Eric
>