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

[Auger Eric]

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.

Thanks

Eric