| Message ID | 20200430143424.2787566-3-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Shared Virtual Addressing for SMMUv3 | expand |
On Thu, 30 Apr 2020 16:34:01 +0200 Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > Let IOASID users take references to existing ioasids with > ioasid_get(). ioasid_free() drops a reference and only frees the > ioasid when its reference number is zero. It returns whether the > ioasid was freed. > Looks good to me, I was planning to do the same for VT-d use. Just a couple of points for potential extension. I can rebase on top of this. > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > include/linux/ioasid.h | 10 ++++++++-- > drivers/iommu/ioasid.c | 30 +++++++++++++++++++++++++++++- > 2 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h > index 6f000d7a0ddcd..609ba6f15b9e3 100644 > --- a/include/linux/ioasid.h > +++ b/include/linux/ioasid.h > @@ -34,7 +34,8 @@ struct ioasid_allocator_ops { > #if IS_ENABLED(CONFIG_IOASID) > ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t > max, void *private); > -void ioasid_free(ioasid_t ioasid); > +void ioasid_get(ioasid_t ioasid); > +bool ioasid_free(ioasid_t ioasid); > void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid, > bool (*getter)(void *)); > int ioasid_register_allocator(struct ioasid_allocator_ops > *allocator); @@ -48,10 +49,15 @@ static inline ioasid_t > ioasid_alloc(struct ioasid_set *set, ioasid_t min, return > INVALID_IOASID; } > > -static inline void ioasid_free(ioasid_t ioasid) > +static inline void ioasid_get(ioasid_t ioasid) > { > } > > +static inline bool ioasid_free(ioasid_t ioasid) > +{ > + return false; > +} > + > static inline void *ioasid_find(struct ioasid_set *set, ioasid_t > ioasid, bool (*getter)(void *)) > { > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c > index 0f8dd377aada3..46511ac53e0c8 100644 > --- a/drivers/iommu/ioasid.c > +++ b/drivers/iommu/ioasid.c > @@ -15,6 +15,7 @@ struct ioasid_data { > struct ioasid_set *set; > void *private; > struct rcu_head rcu; > + refcount_t refs; > }; > > /* > @@ -314,6 +315,7 @@ ioasid_t ioasid_alloc(struct ioasid_set *set, > ioasid_t min, ioasid_t max, > data->set = set; > data->private = private; > + refcount_set(&data->refs, 1); > > /* > * Custom allocator needs allocator data to perform platform > specific @@ -345,12 +347,33 @@ ioasid_t ioasid_alloc(struct > ioasid_set *set, ioasid_t min, ioasid_t max, } > EXPORT_SYMBOL_GPL(ioasid_alloc); > > +/** > + * ioasid_get - obtain a reference to the IOASID > + */ > +void ioasid_get(ioasid_t ioasid) why void? what if the ioasid is not valid. > +{ > + struct ioasid_data *ioasid_data; > + > + spin_lock(&ioasid_allocator_lock); > + ioasid_data = xa_load(&active_allocator->xa, ioasid); > + if (ioasid_data) > + refcount_inc(&ioasid_data->refs); > + spin_unlock(&ioasid_allocator_lock); > +} > +EXPORT_SYMBOL_GPL(ioasid_get); > + > /** > * ioasid_free - Free an IOASID > * @ioasid: the ID to remove > + * > + * Put a reference to the IOASID, free it when the number of > references drops to > + * zero. > + * > + * Return: %true if the IOASID was freed, %false otherwise. > */ > -void ioasid_free(ioasid_t ioasid) > +bool ioasid_free(ioasid_t ioasid) > { > + bool free = false; > struct ioasid_data *ioasid_data; > > spin_lock(&ioasid_allocator_lock); > @@ -360,6 +383,10 @@ void ioasid_free(ioasid_t ioasid) > goto exit_unlock; > } > > + free = refcount_dec_and_test(&ioasid_data->refs); > + if (!free) > + goto exit_unlock; > + Just FYI, we may need to add states for the IOASID, i.g. mark the IOASID inactive after free. And prohibit ioasid_get() after freed. For VT-d, this is useful when KVM queries the IOASID. > active_allocator->ops->free(ioasid, > active_allocator->ops->pdata); /* Custom allocator needs additional > steps to free the xa element */ if (active_allocator->flags & > IOASID_ALLOCATOR_CUSTOM) { @@ -369,6 +396,7 @@ void > ioasid_free(ioasid_t ioasid) > exit_unlock: > spin_unlock(&ioasid_allocator_lock); > + return free; > } > EXPORT_SYMBOL_GPL(ioasid_free); > [Jacob Pan]
On Thu, 30 Apr 2020 11:39:31 -0700 Jacob Pan <jacob.jun.pan@linux.intel.com> wrote: > > -void ioasid_free(ioasid_t ioasid) > > +bool ioasid_free(ioasid_t ioasid) > > { Sorry I missed this in the last reply. I think free needs to be unconditional since there is not a good way to fail it. Also can we have more symmetric APIs, seems we don't have ioasid_put() in this patchset. How about? ioasid_alloc() ioasid_free(); //drop reference, mark inactive, but not reclaimed if refcount is not zero. ioasid_get() // returns err if the ioasid is marked inactive by ioasid_free() ioasid_put();// drop reference, reclaim if refcount is 0. It is similar to get/put/alloc/free pids.
On Thu, Apr 30, 2020 at 11:39:31AM -0700, Jacob Pan wrote: > > +/** > > + * ioasid_get - obtain a reference to the IOASID > > + */ > > +void ioasid_get(ioasid_t ioasid) > why void? what if the ioasid is not valid. My intended use was for the caller to get an additional reference when they're already holding one. So this should always succeed and I'd prefer a WARN_ON if the ioasid isn't valid rather than returning an error. But if you intend to add a state to ioasids between dropping refcount and free, then a return value makes sense. Thanks, Jean > > > +{ > > + struct ioasid_data *ioasid_data; > > + > > + spin_lock(&ioasid_allocator_lock); > > + ioasid_data = xa_load(&active_allocator->xa, ioasid); > > + if (ioasid_data) > > + refcount_inc(&ioasid_data->refs); > > + spin_unlock(&ioasid_allocator_lock); > > +} > > +EXPORT_SYMBOL_GPL(ioasid_get); > > + > > /** > > * ioasid_free - Free an IOASID > > * @ioasid: the ID to remove > > + * > > + * Put a reference to the IOASID, free it when the number of > > references drops to > > + * zero. > > + * > > + * Return: %true if the IOASID was freed, %false otherwise. > > */ > > -void ioasid_free(ioasid_t ioasid) > > +bool ioasid_free(ioasid_t ioasid) > > { > > + bool free = false; > > struct ioasid_data *ioasid_data; > > > > spin_lock(&ioasid_allocator_lock); > > @@ -360,6 +383,10 @@ void ioasid_free(ioasid_t ioasid) > > goto exit_unlock; > > } > > > > + free = refcount_dec_and_test(&ioasid_data->refs); > > + if (!free) > > + goto exit_unlock; > > + > Just FYI, we may need to add states for the IOASID, i.g. mark the IOASID > inactive after free. And prohibit ioasid_get() after freed. For VT-d, > this is useful when KVM queries the IOASID. > > > active_allocator->ops->free(ioasid, > > active_allocator->ops->pdata); /* Custom allocator needs additional > > steps to free the xa element */ if (active_allocator->flags & > > IOASID_ALLOCATOR_CUSTOM) { @@ -369,6 +396,7 @@ void > > ioasid_free(ioasid_t ioasid) > > exit_unlock: > > spin_unlock(&ioasid_allocator_lock); > > + return free; > > } > > EXPORT_SYMBOL_GPL(ioasid_free); > > > > [Jacob Pan]
On Thu, Apr 30, 2020 at 01:48:42PM -0700, Jacob Pan wrote: > On Thu, 30 Apr 2020 11:39:31 -0700 > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote: > > > > -void ioasid_free(ioasid_t ioasid) > > > +bool ioasid_free(ioasid_t ioasid) > > > { > Sorry I missed this in the last reply. > > I think free needs to be unconditional since there is not a good way to > fail it. > > Also can we have more symmetric APIs, seems we don't have ioasid_put() > in this patchset. Yes I was thinking of renaming ioasid_free() to ioasid_put() but got lazy. > How about? > ioasid_alloc() > ioasid_free(); //drop reference, mark inactive, but not reclaimed if > refcount is not zero. > ioasid_get() // returns err if the ioasid is marked inactive by > ioasid_free() How does the caller know that the ioasid is in active/inactive state, and not freed/reallocated? > ioasid_put();// drop reference, reclaim if refcount is 0. I'll add ioasid_put() for now. I'd like to avoid introducing the inactive state in this patch, so shall I change the calls in the Intel driver to ioasid_put(), and not introduce a new ioasid_free() for the moment? Thanks, Jean
On Mon, 4 May 2020 16:39:32 +0200 Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > On Thu, Apr 30, 2020 at 01:48:42PM -0700, Jacob Pan wrote: > > On Thu, 30 Apr 2020 11:39:31 -0700 > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote: > > > > > > -void ioasid_free(ioasid_t ioasid) > > > > +bool ioasid_free(ioasid_t ioasid) > > > > { > > Sorry I missed this in the last reply. > > > > I think free needs to be unconditional since there is not a good > > way to fail it. > > > > Also can we have more symmetric APIs, seems we don't have > > ioasid_put() in this patchset. > > Yes I was thinking of renaming ioasid_free() to ioasid_put() but got > lazy. > > > How about? > > ioasid_alloc() > > ioasid_free(); //drop reference, mark inactive, but not reclaimed if > > refcount is not zero. > > ioasid_get() // returns err if the ioasid is marked inactive by > > ioasid_free() > > How does the caller know that the ioasid is in active/inactive state, > and not freed/reallocated? > In inactive state, callers of ioasid_find(), ioasid_get() would all fail. Only ioasid_put can still operate on it. In freed state (i.e. not allocated), it will be the same as above with the exception that ioasid_put has no effect. > > ioasid_put();// drop reference, reclaim if refcount is 0. > > I'll add ioasid_put() for now. I'd like to avoid introducing the > inactive state in this patch, Sounds good. I just wanted to consult with you about the above APIs. I will introduce the state when we have a real use. > so shall I change the calls in the > Intel driver to ioasid_put(), and not introduce a new ioasid_free() > for the moment? > Sounds good. > Thanks, > Jean > [Jacob Pan]
On Mon, 4 May 2020 16:25:48 +0200 Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > On Thu, Apr 30, 2020 at 11:39:31AM -0700, Jacob Pan wrote: > > > +/** > > > + * ioasid_get - obtain a reference to the IOASID > > > + */ > > > +void ioasid_get(ioasid_t ioasid) > > why void? what if the ioasid is not valid. > > My intended use was for the caller to get an additional reference when > they're already holding one. So this should always succeed and I'd > prefer a WARN_ON if the ioasid isn't valid rather than returning an > error. But if you intend to add a state to ioasids between dropping > refcount and free, then a return value makes sense. > Yes, a WARN_ON will do. No need for return value for now. > Thanks, > Jean > > > > > > +{ > > > + struct ioasid_data *ioasid_data; > > > + > > > + spin_lock(&ioasid_allocator_lock); > > > + ioasid_data = xa_load(&active_allocator->xa, ioasid); > > > + if (ioasid_data) > > > + refcount_inc(&ioasid_data->refs); > > > + spin_unlock(&ioasid_allocator_lock); > > > +} > > > +EXPORT_SYMBOL_GPL(ioasid_get); > > > + > > > /** > > > * ioasid_free - Free an IOASID > > > * @ioasid: the ID to remove > > > + * > > > + * Put a reference to the IOASID, free it when the number of > > > references drops to > > > + * zero. > > > + * > > > + * Return: %true if the IOASID was freed, %false otherwise. > > > */ > > > -void ioasid_free(ioasid_t ioasid) > > > +bool ioasid_free(ioasid_t ioasid) > > > { > > > + bool free = false; > > > struct ioasid_data *ioasid_data; > > > > > > spin_lock(&ioasid_allocator_lock); > > > @@ -360,6 +383,10 @@ void ioasid_free(ioasid_t ioasid) > > > goto exit_unlock; > > > } > > > > > > + free = refcount_dec_and_test(&ioasid_data->refs); > > > + if (!free) > > > + goto exit_unlock; > > > + > > Just FYI, we may need to add states for the IOASID, i.g. mark the > > IOASID inactive after free. And prohibit ioasid_get() after freed. > > For VT-d, this is useful when KVM queries the IOASID. > > > > > active_allocator->ops->free(ioasid, > > > active_allocator->ops->pdata); /* Custom allocator needs > > > additional steps to free the xa element */ if > > > (active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) { @@ -369,6 > > > +396,7 @@ void ioasid_free(ioasid_t ioasid) > > > exit_unlock: > > > spin_unlock(&ioasid_allocator_lock); > > > + return free; > > > } > > > EXPORT_SYMBOL_GPL(ioasid_free); > > > > > > > [Jacob Pan] [Jacob Pan]
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h index 6f000d7a0ddcd..609ba6f15b9e3 100644 --- a/include/linux/ioasid.h +++ b/include/linux/ioasid.h @@ -34,7 +34,8 @@ struct ioasid_allocator_ops { #if IS_ENABLED(CONFIG_IOASID) ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max, void *private); -void ioasid_free(ioasid_t ioasid); +void ioasid_get(ioasid_t ioasid); +bool ioasid_free(ioasid_t ioasid); void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid, bool (*getter)(void *)); int ioasid_register_allocator(struct ioasid_allocator_ops *allocator); @@ -48,10 +49,15 @@ static inline ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, return INVALID_IOASID; } -static inline void ioasid_free(ioasid_t ioasid) +static inline void ioasid_get(ioasid_t ioasid) { } +static inline bool ioasid_free(ioasid_t ioasid) +{ + return false; +} + static inline void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid, bool (*getter)(void *)) { diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c index 0f8dd377aada3..46511ac53e0c8 100644 --- a/drivers/iommu/ioasid.c +++ b/drivers/iommu/ioasid.c @@ -15,6 +15,7 @@ struct ioasid_data { struct ioasid_set *set; void *private; struct rcu_head rcu; + refcount_t refs; }; /* @@ -314,6 +315,7 @@ ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max, data->set = set; data->private = private; + refcount_set(&data->refs, 1); /* * Custom allocator needs allocator data to perform platform specific @@ -345,12 +347,33 @@ ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max, } EXPORT_SYMBOL_GPL(ioasid_alloc); +/** + * ioasid_get - obtain a reference to the IOASID + */ +void ioasid_get(ioasid_t ioasid) +{ + struct ioasid_data *ioasid_data; + + spin_lock(&ioasid_allocator_lock); + ioasid_data = xa_load(&active_allocator->xa, ioasid); + if (ioasid_data) + refcount_inc(&ioasid_data->refs); + spin_unlock(&ioasid_allocator_lock); +} +EXPORT_SYMBOL_GPL(ioasid_get); + /** * ioasid_free - Free an IOASID * @ioasid: the ID to remove + * + * Put a reference to the IOASID, free it when the number of references drops to + * zero. + * + * Return: %true if the IOASID was freed, %false otherwise. */ -void ioasid_free(ioasid_t ioasid) +bool ioasid_free(ioasid_t ioasid) { + bool free = false; struct ioasid_data *ioasid_data; spin_lock(&ioasid_allocator_lock); @@ -360,6 +383,10 @@ void ioasid_free(ioasid_t ioasid) goto exit_unlock; } + free = refcount_dec_and_test(&ioasid_data->refs); + if (!free) + goto exit_unlock; + active_allocator->ops->free(ioasid, active_allocator->ops->pdata); /* Custom allocator needs additional steps to free the xa element */ if (active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) { @@ -369,6 +396,7 @@ void ioasid_free(ioasid_t ioasid) exit_unlock: spin_unlock(&ioasid_allocator_lock); + return free; } EXPORT_SYMBOL_GPL(ioasid_free);
Let IOASID users take references to existing ioasids with ioasid_get(). ioasid_free() drops a reference and only frees the ioasid when its reference number is zero. It returns whether the ioasid was freed. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- include/linux/ioasid.h | 10 ++++++++-- drivers/iommu/ioasid.c | 30 +++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-)