Re: [PATCH v3 03/10] iommu/sva: Manage process address spaces

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi,

On 09/25/2018 06:32 PM, Jean-Philippe Brucker wrote:
> On 25/09/2018 04:15, Lu Baolu wrote:
>>> +     /* If an io_mm already exists, use it */
>>> +     spin_lock(&iommu_sva_lock);
>>> +     idr_for_each_entry(&iommu_pasid_idr, io_mm, i) {
>>
>> This might be problematic for vt-d (and other possible arch's which use
>> PASID other than SVA). When vt-d iommu works in scalable mode, a PASID
>> might be allocated for:
>>
>> (1) SVA
>> (2) Device Assignable Interface (might be a mdev or directly managed
>>       within a device driver).
>> (3) SVA in VM guest
>> (4) Device Assignable Interface in VM guest
>>
>> So we can't expect that an io_mm pointer was associated with each PASID.
> 
> Yes as discussed on the previous series, we'll need to move the PASID
> allocator outside of iommu-sva at some point, and even outside of
> drivers/iommu since device driver might want to use the PASID allocator
> without an IOMMU.
> 
> To be usable by all consumers of PASIDs, that allocator will need the
> same interface as IDR, so I don't think we have a problem here. I
> haven't had time or need to write such allocator yet (and don't plan to
> do it as part of this series), but I drafted an interface, that at least
> fulfills the needs of SVA.

I have a patch set for the global pasid allocator. It mostly matches
your idea. I can send it out for comments later.

Best regards,
Lu Baolu

> 
> * Single system-wide PASID space
> * Multiple consumers, each associating their own structure to PASIDs.
>    Each consumer gets a token.
> * Device drivers might want to use both SVA and private PASIDs for a
>    device at the same time.
> * In my opinion "pasid" isn't the right name, "ioasid" would be better
>    but that's not important.
> 
> typedef unsigned int pasid_t;
> 
> /* Returns consumer token */
> void *pasid_get_consumer();
> void pasid_put_consumer(void *consumer);
> 
> /* Returns pasid or invalid (pasid_t)(-1) */
> pasid_t pasid_alloc(void *consumer, pasid_t min, pasid_t max,
>                      void *private);
> void pasid_remove(pasid_t pasid);
> 
> /* Iterate over PASIDs for this consumer. Func returns non-zero to stop
> iterating */
> int pasid_for_each(void *consumer, void *iter_data,
> 		   int (*func)(void *iter_data, pasid_t pasid,
> 			       void *private));
> /* Returns priv data or NULL */
> void *pasid_find(void *consumer, pasid_t pasid);
> 
> Thanks,
> Jean
> 
>> And this code might run into problem if the pasid is allocated for
>> usages other than SVA.
>>
>> Best regards,
>> Lu Baolu
> 
>