Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On 18/06/2019 18:05, Jacob Pan wrote: > On Tue, 18 Jun 2019 15:22:20 +0100 > Jean-Philippe Brucker wrote: > >> On 11/06/2019 19:13, Jacob Pan wrote: >> +/** >> + * ioasid_find - Find IOASID data >> + * @set: the IOASID set >> + * @ioasid: the IOASID to find >> + * @getter: function to call on the found object >> + * >> + * The optional getter function allows to take a reference to >> the found object >> + * under the rcu lock. The function can also check if the object >> is still valid: >> + * if @getter returns false, then the object is invalid and NULL >> is returned. >> + * >> + * If the IOASID has been allocated for this set, return the >> private pointer >> + * passed to ioasid_alloc. Private data can be NULL if not set. >> Return an error >> + * if the IOASID is not found or does not belong to the set. > > Perhaps should make it clear that @set can be null. Indeed. But I'm not sure allowing @set to be NULL is such a good idea, because the data type associated to an ioasid depends on its set. For example SVA will put an mm_struct in there, and auxiliary domains use some structure private to the IOMMU domain. >>> I am not sure we need to count on @set to decipher data type. >>> Whoever does the allocation and owns the IOASID should knows its >>> own data type. My thought was that @set is only used to group IDs, >>> permission check etc. >>> Jacob, could me make @set mandatory, or do you see a use for a global search? If @set is NULL, then callers can check if the return pointer is NULL, but will run into trouble if they try to dereference it. >>> A global search use case can be for PRQ. IOMMU driver gets a IOASID >>> (first interrupt then retrieve from a queue), it has no idea which >>> @set it belongs to. But the data types are the same for all IOASIDs >>> used by the IOMMU. >> >> They aren't when we use a generic SVA handler. Following a call to >> iommu_sva_bind_device(), iommu-sva.c allocates an IOASID and store an >> mm_struct. If auxiliary domains are also enabled for the device, >> following a call to iommu_aux_attach_device() the IOMMU driver >> allocates an IOASID and stores some private object. >> >> Now for example the IOMMU driver receives a PPR and calls >> ioasid_find() with @set = NULL. ioasid_find() may return either an >> mm_struct or a private object, and the driver cannot know which it is >> so the returned value is unusable. > I think we might have a misunderstanding of what ioasid_set is. Or i > have misused your original intention for it:) So my understanding of an > ioasid_set is to identify a sub set of IOASIDs that > 1. shares the same data type > 2. belongs to the same permission group, owner. In my case it's mostly #1. The two IOASID sets (SVA and AUX) are managed by different modules (iommu-sva and arm-smmu-v3). Since we don't do scalable IOV, the two sets are shared_ioasid and private_ioasid, with either an mm or NULL as private data (but we might need to store a domain for private IOASIDs at some point). So at the moment passing a NULL set to ioasid_find() would work for us as well. However in a non-virtualization scenario, a device driver could define its own set and allocate an IOASID for its own use, associating some private structure with it. If it somehow causes a PRQ on that IOASID, the IOMMU driver shouldn't attempt to access the device driver's private structure. So I do think we need to be careful with global ioasid_find(). Given that any driver in the system can use the allocator, we won't be able to keep assuming that all objects stored in there are of one type. > Our usage is #2., we put a mm_struct as the set to do permission > check. E.g VFIO can check guest PASID ownership based on QEMU process > mm. This will make sure IOASID allocated by one guest can only be used > by the same guest. > > For IOASID used for sva bind, let it be native or guest sva, we always > have the same data type. Therefore, when page request handler gets > called to search with ioasid_find(), it knows its data type. An > additional flag will tell if it is a guest bind or native bind. > > I was under the assumption that aux domain and its IOASID is a 1:1 > mapping, there is no need for a search. Or even it needs to search, it > will be searched by the same caller that did the allocation, therefore > it knows what private data type is. > > In addition, aux domain is used for request w/o PASID. And PPR for > request w/o PASID is not to be supported. So there would not be a need > to search from page request handler. > > Now if we take the above assumption away, and use ioasid_set strictly > to differentiate the data types (Usage #1). Then I agree we can get > rid of NULL set and global search. > > That would require we converge on the generic sva_bind for both > native and guest. The additional implication is that VFIO layer has to >
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On Tue, 18 Jun 2019 15:22:20 +0100 Jean-Philippe Brucker wrote: > On 11/06/2019 19:13, Jacob Pan wrote: > +/** > + * ioasid_find - Find IOASID data > + * @set: the IOASID set > + * @ioasid: the IOASID to find > + * @getter: function to call on the found object > + * > + * The optional getter function allows to take a reference to > the found object > + * under the rcu lock. The function can also check if the object > is still valid: > + * if @getter returns false, then the object is invalid and NULL > is returned. > + * > + * If the IOASID has been allocated for this set, return the > private pointer > + * passed to ioasid_alloc. Private data can be NULL if not set. > Return an error > + * if the IOASID is not found or does not belong to the set. > >>> > >>> Perhaps should make it clear that @set can be null. > >> > >> Indeed. But I'm not sure allowing @set to be NULL is such a good > >> idea, because the data type associated to an ioasid depends on its > >> set. For example SVA will put an mm_struct in there, and auxiliary > >> domains use some structure private to the IOMMU domain. > >> > > I am not sure we need to count on @set to decipher data type. > > Whoever does the allocation and owns the IOASID should knows its > > own data type. My thought was that @set is only used to group IDs, > > permission check etc. > > > >> Jacob, could me make @set mandatory, or do you see a use for a > >> global search? If @set is NULL, then callers can check if the > >> return pointer is NULL, but will run into trouble if they try to > >> dereference it. > > A global search use case can be for PRQ. IOMMU driver gets a IOASID > > (first interrupt then retrieve from a queue), it has no idea which > > @set it belongs to. But the data types are the same for all IOASIDs > > used by the IOMMU. > > They aren't when we use a generic SVA handler. Following a call to > iommu_sva_bind_device(), iommu-sva.c allocates an IOASID and store an > mm_struct. If auxiliary domains are also enabled for the device, > following a call to iommu_aux_attach_device() the IOMMU driver > allocates an IOASID and stores some private object. > > Now for example the IOMMU driver receives a PPR and calls > ioasid_find() with @set = NULL. ioasid_find() may return either an > mm_struct or a private object, and the driver cannot know which it is > so the returned value is unusable. I think we might have a misunderstanding of what ioasid_set is. Or i have misused your original intention for it:) So my understanding of an ioasid_set is to identify a sub set of IOASIDs that 1. shares the same data type 2. belongs to the same permission group, owner. Our usage is #2., we put a mm_struct as the set to do permission check. E.g VFIO can check guest PASID ownership based on QEMU process mm. This will make sure IOASID allocated by one guest can only be used by the same guest. For IOASID used for sva bind, let it be native or guest sva, we always have the same data type. Therefore, when page request handler gets called to search with ioasid_find(), it knows its data type. An additional flag will tell if it is a guest bind or native bind. I was under the assumption that aux domain and its IOASID is a 1:1 mapping, there is no need for a search. Or even it needs to search, it will be searched by the same caller that did the allocation, therefore it knows what private data type is. In addition, aux domain is used for request w/o PASID. And PPR for request w/o PASID is not to be supported. So there would not be a need to search from page request handler. Now if we take the above assumption away, and use ioasid_set strictly to differentiate the data types (Usage #1). Then I agree we can get rid of NULL set and global search. That would require we converge on the generic sva_bind for both native and guest. The additional implication is that VFIO layer has to be SVA struct aware in order to sanitize the mm_struct for guest bind. i.e. check guest ownership by using QEMU process mm. Whereas we have today, VFIO just use IOASID set as mm to check ownership, no need to know the type. Can we keep the NULL set for now and remove it __after__ we converge on the sva bind flows? Thanks, Jacob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On 11/06/2019 19:13, Jacob Pan wrote: +/** + * ioasid_find - Find IOASID data + * @set: the IOASID set + * @ioasid: the IOASID to find + * @getter: function to call on the found object + * + * The optional getter function allows to take a reference to the found object + * under the rcu lock. The function can also check if the object is still valid: + * if @getter returns false, then the object is invalid and NULL is returned. + * + * If the IOASID has been allocated for this set, return the private pointer + * passed to ioasid_alloc. Private data can be NULL if not set. Return an error + * if the IOASID is not found or does not belong to the set. >>> >>> Perhaps should make it clear that @set can be null. >> >> Indeed. But I'm not sure allowing @set to be NULL is such a good idea, >> because the data type associated to an ioasid depends on its set. For >> example SVA will put an mm_struct in there, and auxiliary domains use >> some structure private to the IOMMU domain. >> > I am not sure we need to count on @set to decipher data type. Whoever > does the allocation and owns the IOASID should knows its own data type. > My thought was that @set is only used to group IDs, permission check > etc. > >> Jacob, could me make @set mandatory, or do you see a use for a global >> search? If @set is NULL, then callers can check if the return pointer >> is NULL, but will run into trouble if they try to dereference it. >> > A global search use case can be for PRQ. IOMMU driver gets a IOASID > (first interrupt then retrieve from a queue), it has no idea which > @set it belongs to. But the data types are the same for all IOASIDs > used by the IOMMU. They aren't when we use a generic SVA handler. Following a call to iommu_sva_bind_device(), iommu-sva.c allocates an IOASID and store an mm_struct. If auxiliary domains are also enabled for the device, following a call to iommu_aux_attach_device() the IOMMU driver allocates an IOASID and stores some private object. Now for example the IOMMU driver receives a PPR and calls ioasid_find() with @set = NULL. ioasid_find() may return either an mm_struct or a private object, and the driver cannot know which it is so the returned value is unusable. Thanks, Jean
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On 11/06/2019 18:10, Jacob Pan wrote: >> The issue is theoretical at the moment because no users do this, but >> I'd be more comfortable taking the xa_lock, which prevents a >> concurrent xa_erase()+free(). (I commented on your v3 but you might >> have missed it) >> > Did you reply to my v3? I did not see it. I only saw your comments about > v3 in your commit message. My fault, I sneaked the comments in a random reply three levels down the thread: https://lore.kernel.org/linux-iommu/836caf0d-699e-33ba-5303-b1c9c949c...@arm.com/ (Great, linux-iommu is indexed by lore! I won't have to Cc lkml anymore) + ioasid_data = xa_load(_xa, ioasid); + if (ioasid_data) + rcu_assign_pointer(ioasid_data->private, data); >>> it is good to publish and have barrier here. But I just wonder even >>> for weakly ordered machine, this pointer update is quite far away >>> from its data update. >> >> I don't know, it could be right before calling ioasid_set_data(): >> >> mydata = kzalloc(sizeof(*mydata)); >> mydata->ops = _ops; (1) >> ioasid_set_data(ioasid, mydata); >> ... /* no write barrier here */ >> data->private = mydata; (2) >> >> And then another thread calls ioasid_find(): >> >> mydata = ioasid_find(ioasid); >> if (mydata) >> mydata->ops->do_something(); >> >> On a weakly ordered machine, this thread could observe the pointer >> assignment (2) before the ops assignment (1), and dereference NULL. >> Using rcu_assign_pointer() should fix that >> > I agree it is better to have the barrier. Just thought there is already > a rcu_read_lock() in xa_load() in between. rcu_read_lock() may have > barrier in some case but better not count on it. Yes, and even if rcu_read_lock() provided a barrier I don't think it would be sufficient, because acquire semantics don't guarantee that prior writes appear to happen before the barrier, only the other way round. A lock operation with release semantics, for example spin_unlock(), should work. Thanks, Jean > No issues here. I will > integrate this in the next version. > >> Thanks, >> Jean > > [Jacob Pan] >
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On Tue, 11 Jun 2019 15:35:22 +0100
Jean-Philippe Brucker wrote:
> On 11/06/2019 10:36, Jonathan Cameron wrote:
> >> +/**
> >> + * ioasid_alloc - Allocate an IOASID
> >> + * @set: the IOASID set
> >> + * @min: the minimum ID (inclusive)
> >> + * @max: the maximum ID (inclusive)
> >> + * @private: data private to the caller
> >> + *
> >> + * Allocate an ID between @min and @max. The @private pointer is
> >> stored
> >> + * internally and can be retrieved with ioasid_find().
> >> + *
> >> + * Return: the allocated ID on success, or %INVALID_IOASID on
> >> failure.
> >> + */
> >> +ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
> >> ioasid_t max,
> >> +void *private)
> >> +{
> >> + u32 id = INVALID_IOASID;
> >> + struct ioasid_data *data;
> >> +
> >> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> >> + if (!data)
> >> + return INVALID_IOASID;
> >> +
> >> + data->set = set;
> >> + data->private = private;
> >> +
> >> + if (xa_alloc(_xa, , data, XA_LIMIT(min, max),
> >> GFP_KERNEL)) {
> >> + pr_err("Failed to alloc ioasid from %d to %d\n",
> >> min, max);
> >> + goto exit_free;
> >> + }
> >> + data->id = id;
> >> +
> >> +exit_free:
> >
> > This error flow is perhaps a little more confusing than it needs to
> > be?
> >
> > My assumption (perhaps wrong) is that we only have an id ==
> > INVALID_IOASID if the xa_alloc fails, and that we will always have
> > such an id value if it does (I'm not totally sure this second
> > element is true in __xa_alloc).
> >
> > If I'm missing something perhaps a comment on how else we'd get
> > here.
>
> Yes we can simplify this:
>
> return id;
> exit_free:
> kfree(data)
> return INVALID_IOASID;
> }
>
> The XA API doesn't say that @id passed to xa_alloc() won't be modified
> in case of error. It's true in the current implementation, but won't
> necessarily stay that way. On the other hand I think it's safe to
> expect @id to always be set when xa_alloc() succeeds.
>
the flow with custom allocator is slightly different, but you are right
I can simplify it as you suggested.
Jonathan, I will add you to the cc list in next version. If you could
also review the current version, it would be greatly appreciated.
https://lore.kernel.org/lkml/1560087862-57608-13-git-send-email-jacob.jun@linux.intel.com/
> >> +/**
> >> + * ioasid_find - Find IOASID data
> >> + * @set: the IOASID set
> >> + * @ioasid: the IOASID to find
> >> + * @getter: function to call on the found object
> >> + *
> >> + * The optional getter function allows to take a reference to the
> >> found object
> >> + * under the rcu lock. The function can also check if the object
> >> is still valid:
> >> + * if @getter returns false, then the object is invalid and NULL
> >> is returned.
> >> + *
> >> + * If the IOASID has been allocated for this set, return the
> >> private pointer
> >> + * passed to ioasid_alloc. Private data can be NULL if not set.
> >> Return an error
> >> + * if the IOASID is not found or does not belong to the set.
> >
> > Perhaps should make it clear that @set can be null.
>
> Indeed. But I'm not sure allowing @set to be NULL is such a good idea,
> because the data type associated to an ioasid depends on its set. For
> example SVA will put an mm_struct in there, and auxiliary domains use
> some structure private to the IOMMU domain.
>
I am not sure we need to count on @set to decipher data type. Whoever
does the allocation and owns the IOASID should knows its own data type.
My thought was that @set is only used to group IDs, permission check
etc.
> Jacob, could me make @set mandatory, or do you see a use for a global
> search? If @set is NULL, then callers can check if the return pointer
> is NULL, but will run into trouble if they try to dereference it.
>
A global search use case can be for PRQ. IOMMU driver gets a IOASID
(first interrupt then retrieve from a queue), it has no idea which
@set it belongs to. But the data types are the same for all IOASIDs
used by the IOMMU.
If @set is NULL, the search does not check set match. It is separate
from return pointer. Sorry i am not seeing the problems here.
> >
> >> + */
> >> +void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
> >> +bool (*getter)(void *))
> >> +{
> >> + void *priv = NULL;
> >
> > Set in all paths, so does need to be set here.
>
> Right
>
> Thanks,
> Jean
[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On Tue, 11 Jun 2019 15:37:42 +0100
Jean-Philippe Brucker wrote:
> On 11/06/2019 13:26, Jacob Pan wrote:
> >> +/**
> >> + * ioasid_set_data - Set private data for an allocated ioasid
> >> + * @ioasid: the ID to set data
> >> + * @data: the private data
> >> + *
> >> + * For IOASID that is already allocated, private data can be set
> >> + * via this API. Future lookup can be done via ioasid_find.
> >> + */
> >> +int ioasid_set_data(ioasid_t ioasid, void *data)
> >> +{
> >> + struct ioasid_data *ioasid_data;
> >> + int ret = 0;
> >> +
> >> + xa_lock(_xa);
> > Just wondering if this is necessary, since xa_load is under
> > rcu_read_lock and we are not changing anything internal to xa. For
> > custom allocator I still need to have the mutex against allocator
> > removal.
>
> I think we do need this because of a possible race with ioasid_free():
>
> CPU1 CPU2
> ioasid_free(ioasid) ioasid_set_data(ioasid, foo)
> data = xa_load(...)
> xa_erase(...)
> kfree_rcu(data) (no RCU lock held)
> ...free(data)
> data->private = foo;
>
make sense, thanks for explaining.
> The issue is theoretical at the moment because no users do this, but
> I'd be more comfortable taking the xa_lock, which prevents a
> concurrent xa_erase()+free(). (I commented on your v3 but you might
> have missed it)
>
Did you reply to my v3? I did not see it. I only saw your comments about
v3 in your commit message.
> >> + ioasid_data = xa_load(_xa, ioasid);
> >> + if (ioasid_data)
> >> + rcu_assign_pointer(ioasid_data->private, data);
> > it is good to publish and have barrier here. But I just wonder even
> > for weakly ordered machine, this pointer update is quite far away
> > from its data update.
>
> I don't know, it could be right before calling ioasid_set_data():
>
> mydata = kzalloc(sizeof(*mydata));
> mydata->ops = _ops; (1)
> ioasid_set_data(ioasid, mydata);
> ... /* no write barrier here */
> data->private = mydata; (2)
>
> And then another thread calls ioasid_find():
>
> mydata = ioasid_find(ioasid);
> if (mydata)
> mydata->ops->do_something();
>
> On a weakly ordered machine, this thread could observe the pointer
> assignment (2) before the ops assignment (1), and dereference NULL.
> Using rcu_assign_pointer() should fix that
>
I agree it is better to have the barrier. Just thought there is already
a rcu_read_lock() in xa_load() in between. rcu_read_lock() may have
barrier in some case but better not count on it. No issues here. I will
integrate this in the next version.
> Thanks,
> Jean
[Jacob Pan]
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On 11/06/2019 13:26, Jacob Pan wrote:
>> +/**
>> + * ioasid_set_data - Set private data for an allocated ioasid
>> + * @ioasid: the ID to set data
>> + * @data: the private data
>> + *
>> + * For IOASID that is already allocated, private data can be set
>> + * via this API. Future lookup can be done via ioasid_find.
>> + */
>> +int ioasid_set_data(ioasid_t ioasid, void *data)
>> +{
>> +struct ioasid_data *ioasid_data;
>> +int ret = 0;
>> +
>> +xa_lock(_xa);
> Just wondering if this is necessary, since xa_load is under
> rcu_read_lock and we are not changing anything internal to xa. For
> custom allocator I still need to have the mutex against allocator
> removal.
I think we do need this because of a possible race with ioasid_free():
CPU1 CPU2
ioasid_free(ioasid) ioasid_set_data(ioasid, foo)
data = xa_load(...)
xa_erase(...)
kfree_rcu(data) (no RCU lock held)
...free(data)
data->private = foo;
The issue is theoretical at the moment because no users do this, but I'd
be more comfortable taking the xa_lock, which prevents a concurrent
xa_erase()+free(). (I commented on your v3 but you might have missed it)
>> +ioasid_data = xa_load(_xa, ioasid);
>> +if (ioasid_data)
>> +rcu_assign_pointer(ioasid_data->private, data);
> it is good to publish and have barrier here. But I just wonder even for
> weakly ordered machine, this pointer update is quite far away from its
> data update.
I don't know, it could be right before calling ioasid_set_data():
mydata = kzalloc(sizeof(*mydata));
mydata->ops = _ops; (1)
ioasid_set_data(ioasid, mydata);
... /* no write barrier here */
data->private = mydata; (2)
And then another thread calls ioasid_find():
mydata = ioasid_find(ioasid);
if (mydata)
mydata->ops->do_something();
On a weakly ordered machine, this thread could observe the pointer
assignment (2) before the ops assignment (1), and dereference NULL.
Using rcu_assign_pointer() should fix that
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On 11/06/2019 10:36, Jonathan Cameron wrote:
>> +/**
>> + * ioasid_alloc - Allocate an IOASID
>> + * @set: the IOASID set
>> + * @min: the minimum ID (inclusive)
>> + * @max: the maximum ID (inclusive)
>> + * @private: data private to the caller
>> + *
>> + * Allocate an ID between @min and @max. The @private pointer is stored
>> + * internally and can be retrieved with ioasid_find().
>> + *
>> + * Return: the allocated ID on success, or %INVALID_IOASID on failure.
>> + */
>> +ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max,
>> + void *private)
>> +{
>> +u32 id = INVALID_IOASID;
>> +struct ioasid_data *data;
>> +
>> +data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +if (!data)
>> +return INVALID_IOASID;
>> +
>> +data->set = set;
>> +data->private = private;
>> +
>> +if (xa_alloc(_xa, , data, XA_LIMIT(min, max), GFP_KERNEL)) {
>> +pr_err("Failed to alloc ioasid from %d to %d\n", min, max);
>> +goto exit_free;
>> +}
>> +data->id = id;
>> +
>> +exit_free:
>
> This error flow is perhaps a little more confusing than it needs to be?
>
> My assumption (perhaps wrong) is that we only have an id == INVALID_IOASID
> if the xa_alloc fails, and that we will always have such an id value if
> it does (I'm not totally sure this second element is true in __xa_alloc).
>
> If I'm missing something perhaps a comment on how else we'd get here.
Yes we can simplify this:
return id;
exit_free:
kfree(data)
return INVALID_IOASID;
}
The XA API doesn't say that @id passed to xa_alloc() won't be modified
in case of error. It's true in the current implementation, but won't
necessarily stay that way. On the other hand I think it's safe to expect
@id to always be set when xa_alloc() succeeds.
>> +/**
>> + * ioasid_find - Find IOASID data
>> + * @set: the IOASID set
>> + * @ioasid: the IOASID to find
>> + * @getter: function to call on the found object
>> + *
>> + * The optional getter function allows to take a reference to the found
>> object
>> + * under the rcu lock. The function can also check if the object is still
>> valid:
>> + * if @getter returns false, then the object is invalid and NULL is
>> returned.
>> + *
>> + * If the IOASID has been allocated for this set, return the private pointer
>> + * passed to ioasid_alloc. Private data can be NULL if not set. Return an
>> error
>> + * if the IOASID is not found or does not belong to the set.
>
> Perhaps should make it clear that @set can be null.
Indeed. But I'm not sure allowing @set to be NULL is such a good idea,
because the data type associated to an ioasid depends on its set. For
example SVA will put an mm_struct in there, and auxiliary domains use
some structure private to the IOMMU domain.
Jacob, could me make @set mandatory, or do you see a use for a global
search? If @set is NULL, then callers can check if the return pointer is
NULL, but will run into trouble if they try to dereference it.
>
>> + */
>> +void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
>> + bool (*getter)(void *))
>> +{
>> +void *priv = NULL;
>
> Set in all paths, so does need to be set here.
Right
Thanks,
Jean
Re: [PATCH 1/8] iommu: Add I/O ASID allocator
On Mon, 10 Jun 2019 19:47:07 +0100
Jean-Philippe Brucker wrote:
> Some devices might support multiple DMA address spaces, in particular
> those that have the PCI PASID feature. PASID (Process Address Space
> ID) allows to share process address spaces with devices (SVA),
> partition a device into VM-assignable entities (VFIO mdev) or simply
> provide multiple DMA address space to kernel drivers. Add a global
> PASID allocator usable by different drivers at the same time. Name it
> I/O ASID to avoid confusion with ASIDs allocated by arch code, which
> are usually a separate ID space.
>
> The IOASID space is global. Each device can have its own PASID space,
> but by convention the IOMMU ended up having a global PASID space, so
> that with SVA, each mm_struct is associated to a single PASID.
>
> The allocator is primarily used by IOMMU subsystem but in rare
> occasions drivers would like to allocate PASIDs for devices that
> aren't managed by an IOMMU, using the same ID space as IOMMU.
>
> Signed-off-by: Jean-Philippe Brucker
> Signed-off-by: Jacob Pan
> ---
> The most recent discussion on this patch was at:
> https://lkml.kernel.org/lkml/1556922737-76313-4-git-send-email-jacob.jun@linux.intel.com/
> I fixed it up a bit following comments in that series, and removed the
> definitions for the custom allocator for now.
>
> There also is a new version that includes the custom allocator into
> this patch, but is currently missing the RCU fixes, at:
> https://lore.kernel.org/lkml/1560087862-57608-13-git-send-email-jacob.jun@linux.intel.com/
> ---
> drivers/iommu/Kconfig | 4 ++
> drivers/iommu/Makefile | 1 +
> drivers/iommu/ioasid.c | 150
> + include/linux/ioasid.h |
> 49 ++ 4 files changed, 204 insertions(+)
> create mode 100644 drivers/iommu/ioasid.c
> create mode 100644 include/linux/ioasid.h
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 83664db5221d..9b45f70549a7 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -3,6 +3,10 @@
> config IOMMU_IOVA
> tristate
>
> +# The IOASID library may also be used by non-IOMMU_API users
> +config IOASID
> + tristate
> +
> # IOMMU_API always gets selected by whoever wants it.
> config IOMMU_API
> bool
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 8c71a15e986b..0efac6f1ec73 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
> +obj-$(CONFIG_IOASID) += ioasid.o
> obj-$(CONFIG_IOMMU_IOVA) += iova.o
> obj-$(CONFIG_OF_IOMMU) += of_iommu.o
> obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> new file mode 100644
> index ..bbb771214fa9
> --- /dev/null
> +++ b/drivers/iommu/ioasid.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * I/O Address Space ID allocator. There is one global IOASID space,
> split into
> + * subsets. Users create a subset with DECLARE_IOASID_SET, then
> allocate and
> + * free IOASIDs with ioasid_alloc and ioasid_free.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct ioasid_data {
> + ioasid_t id;
> + struct ioasid_set *set;
> + void *private;
> + struct rcu_head rcu;
> +};
> +
> +static DEFINE_XARRAY_ALLOC(ioasid_xa);
> +
> +/**
> + * ioasid_set_data - Set private data for an allocated ioasid
> + * @ioasid: the ID to set data
> + * @data: the private data
> + *
> + * For IOASID that is already allocated, private data can be set
> + * via this API. Future lookup can be done via ioasid_find.
> + */
> +int ioasid_set_data(ioasid_t ioasid, void *data)
> +{
> + struct ioasid_data *ioasid_data;
> + int ret = 0;
> +
> + xa_lock(_xa);
Just wondering if this is necessary, since xa_load is under
rcu_read_lock and we are not changing anything internal to xa. For
custom allocator I still need to have the mutex against allocator
removal.
> + ioasid_data = xa_load(_xa, ioasid);
> + if (ioasid_data)
> + rcu_assign_pointer(ioasid_data->private, data);
it is good to publish and have barrier here. But I just wonder even for
weakly ordered machine, this pointer update is quite far away from its
data update.
> + else
> + ret = -ENOENT;
> + xa_unlock(_xa);
> +
> + /*
> + * Wait for readers to stop accessing the old private data,
> so the
> + * caller can free it.
> + */
> + if (!ret)
> + synchronize_rcu();
> +
I will add that to my next version to check ret value.
Thanks,
Jacob
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_set_data);
> +
> +/**
> + * ioasid_alloc - Allocate an IOASID
> + * @set: the IOASID set
> + * @min: the
