Re: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API

["Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>]

Am 06.09.2018 um 14:45 schrieb Jean-Philippe Brucker:
> On 06/09/2018 12:12, Christian König wrote:
>> Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker:
>>> Hi Eric,
>>>
>>> Thanks for reviewing
>>>
>>> On 05/09/2018 12:29, Auger Eric wrote:
>>>>> +int iommu_sva_device_init(struct device *dev, unsigned long features,
>>>>> +              unsigned int max_pasid)
>>>> what about min_pasid?
>>> No one asked for it... The max_pasid parameter is here for drivers that
>>> have vendor-specific PASID size limits, such as AMD KFD (see
>>> kfd_iommu_device_init and
>>> https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases
>>> the PASID size will only depend on the PCI PASID capability and the
>>> IOMMU limits, both known by the IOMMU driver, so device drivers won't
>>> have to set max_pasid.
>>>
>>> IOMMU drivers need to set min_pasid in the sva_device_init callback
>>> because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0
>>> (Intel Vt-d rev2), but at the moment I can't see a reason for device
>>> drivers to override min_pasid
>> Sorry to ruin your day, but if I'm not completely mistaken PASID zero is
>> reserved in the AMD KFD as well.
> Heh, fair enough. I'll add the min_pasid parameter

I will take this as an opportunity to summarize some of the requirements 
we have for PASID management from the amdgpu driver point of view:

1. We need to be able to allocate PASID between 1 and some maximum. Zero 
is reserved as far as I know, but we don't necessary need a minimum.

2. We need to be able to allocate PASIDs without a process address space 
backing it. E.g. our hardware uses PASIDs even without Shared Virtual 
Addressing enabled to distinct clients from each other.
         Would be a pity if we need to still have a separate PASID 
handling because the system wide is only available when IOMMU is turned on.

3. Even after destruction of a process address space we need some grace 
period before a PASID is reused because it can be that the specific 
PASID is still in some hardware queues etc...
         At bare minimum all device drivers using process binding need 
to explicitly note to the core when they are done with a PASID.

4. It would be nice to have to be able to set a "void *" for each 
PASID/device combination while binding to a process which then can be 
queried later on based on the PASID.
         E.g. when you have a per PASID/device structure around anyway, 
just add an extra field.

5. It would be nice to have to allocate multiple PASIDs for the same 
process address space.
         E.g. some teams at AMD want to use a separate GPU address space 
for their userspace client library. I'm still trying to avoid that, but 
it is perfectly possible that we are going to need that.
         Additional to that it is sometimes quite useful for debugging 
to isolate where exactly an incorrect access (segfault) is coming from.

Let me know if there are some problems with that, especially I want to 
know if there is pushback on #5 so that I can forward that :)

Thanks,
Christian.

>
> Thanks,
> Jean

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu