Re: [PATCH v3 04/16] ioasid: Add custom IOASID allocator
On Thu, 23 May 2019 09:14:07 +0200
Auger Eric wrote:
> Hi Jacob,
>
> On 5/22/19 9:42 PM, Jacob Pan wrote:
> > On Tue, 21 May 2019 11:55:55 +0200
> > Auger Eric wrote:
> >
> >> Hi Jacob,
> >>
> >> On 5/4/19 12:32 AM, Jacob Pan wrote:
> >>> Sometimes, IOASID allocation must be handled by platform specific
> >>> code. The use cases are guest vIOMMU and pvIOMMU where IOASIDs
> >>> need to be allocated by the host via enlightened or paravirt
> >>> interfaces.
> >>>
> >>> This patch adds an extension to the IOASID allocator APIs such
> >>> that platform drivers can register a custom allocator, possibly
> >>> at boot time, to take over the allocation. Xarray is still used
> >>> for tracking and searching purposes internal to the IOASID code.
> >>> Private data of an IOASID can also be set after the allocation.
> >>>
> >>> There can be multiple custom allocators registered but only one is
> >>> used at a time. In case of hot removal of devices that provides
> >>> the allocator, all IOASIDs must be freed prior to unregistering
> >>> the allocator. Default XArray based allocator cannot be mixed with
> >>> custom allocators, i.e. custom allocators will not be used if
> >>> there are outstanding IOASIDs allocated by the default XA
> >>> allocator.
> >>>
> >>> Signed-off-by: Jacob Pan
> >>> ---
> >>> drivers/iommu/ioasid.c | 125
> >>> + 1 file changed,
> >>> 125 insertions(+)
> >>>
> >>> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> >>> index 99f5e0a..ed2915a 100644
> >>> --- a/drivers/iommu/ioasid.c
> >>> +++ b/drivers/iommu/ioasid.c
> >>> @@ -17,6 +17,100 @@ struct ioasid_data {
> >>> };
> >>>
> >>> static DEFINE_XARRAY_ALLOC(ioasid_xa);
> >>> +static DEFINE_MUTEX(ioasid_allocator_lock);
> >>> +static struct ioasid_allocator *active_custom_allocator;
> >>> +
> >>> +static LIST_HEAD(custom_allocators);
> >>> +/*
> >>> + * A flag to track if ioasid default allocator is in use, this
> >>> will
> >>> + * prevent custom allocator from being used. The reason is that
> >>> custom allocator
> >>> + * must have unadulterated space to track private data with
> >>> xarray, there cannot
> >>> + * be a mix been default and custom allocated IOASIDs.
> >>> + */
> >>> +static int default_allocator_active;
> >>> +
> >>> +/**
> >>> + * ioasid_register_allocator - register a custom allocator
> >>> + * @allocator: the custom allocator to be registered
> >>> + *
> >>> + * Custom allocators take precedence over the default xarray
> >>> based allocator.
> >>> + * Private data associated with the ASID are managed by ASID
> >>> common code
> >>> + * similar to data stored in xa.
> >>> + *
> >>> + * There can be multiple allocators registered but only one is
> >>> active. In case
> >>> + * of runtime removal of a custom allocator, the next one is
> >>> activated based
> >>> + * on the registration ordering.
> >>> + */
> >>> +int ioasid_register_allocator(struct ioasid_allocator *allocator)
> >>> +{
> >>> + struct ioasid_allocator *pallocator;
> >>> + int ret = 0;
> >>> +
> >>> + if (!allocator)
> >>> + return -EINVAL;
> >> is it really necessary? Sin't it the caller responsibility?
> > makes sense. will remove this one and below.
> >>> +
> >>> + mutex_lock(_allocator_lock);
> >>> + /*
> >>> + * No particular preference since all custom allocators
> >>> end up calling
> >>> + * the host to allocate IOASIDs. We activate the first
> >>> one and keep
> >>> + * the later registered allocators in a list in case the
> >>> first one gets
> >>> + * removed due to hotplug.
> >>> + */
> >>> + if (list_empty(_allocators))
> >>> + active_custom_allocator = allocator;> +
> >>> else {
> >>> + /* Check if the allocator is already registered
> >>> */
> >>> + list_for_each_entry(pallocator,
> >>> _allocators, list) {
> >>> + if (pallocator == allocator) {
> >>> + pr_err("IOASID allocator already
> >>> registered\n");
> >>> + ret = -EEXIST;
> >>> + goto out_unlock;
> >>> + }
> >>> + }
> >>> + }
> >>> + list_add_tail(>list, _allocators);
> >>> +
> >>> +out_unlock:
> >>> + mutex_unlock(_allocator_lock);
> >>> + return ret;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(ioasid_register_allocator);
> >>> +
> >>> +/**
> >>> + * ioasid_unregister_allocator - Remove a custom IOASID allocator
> >>> + * @allocator: the custom allocator to be removed
> >>> + *
> >>> + * Remove an allocator from the list, activate the next allocator
> >>> in
> >>> + * the order it was registered.
> >>> + */
> >>> +void ioasid_unregister_allocator(struct ioasid_allocator
> >>> *allocator) +{
> >>> + if (!allocator)
> >>> + return;
> >> is it really necessary?
> >>> +
> >>> + if (list_empty(_allocators)) {
> >>> + pr_warn("No custom IOASID allocators active!\n");
> >>> + return;
> >>> + }
> >>> +
> >>> + mutex_lock(_allocator_lock);
> >>> +
Re: [PATCH v3 04/16] ioasid: Add custom IOASID allocator
Hi Jacob,
On 5/22/19 9:42 PM, Jacob Pan wrote:
> On Tue, 21 May 2019 11:55:55 +0200
> Auger Eric wrote:
>
>> Hi Jacob,
>>
>> On 5/4/19 12:32 AM, Jacob Pan wrote:
>>> Sometimes, IOASID allocation must be handled by platform specific
>>> code. The use cases are guest vIOMMU and pvIOMMU where IOASIDs need
>>> to be allocated by the host via enlightened or paravirt interfaces.
>>>
>>> This patch adds an extension to the IOASID allocator APIs such that
>>> platform drivers can register a custom allocator, possibly at boot
>>> time, to take over the allocation. Xarray is still used for tracking
>>> and searching purposes internal to the IOASID code. Private data of
>>> an IOASID can also be set after the allocation.
>>>
>>> There can be multiple custom allocators registered but only one is
>>> used at a time. In case of hot removal of devices that provides the
>>> allocator, all IOASIDs must be freed prior to unregistering the
>>> allocator. Default XArray based allocator cannot be mixed with
>>> custom allocators, i.e. custom allocators will not be used if there
>>> are outstanding IOASIDs allocated by the default XA allocator.
>>>
>>> Signed-off-by: Jacob Pan
>>> ---
>>> drivers/iommu/ioasid.c | 125
>>> + 1 file changed,
>>> 125 insertions(+)
>>>
>>> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
>>> index 99f5e0a..ed2915a 100644
>>> --- a/drivers/iommu/ioasid.c
>>> +++ b/drivers/iommu/ioasid.c
>>> @@ -17,6 +17,100 @@ struct ioasid_data {
>>> };
>>>
>>> static DEFINE_XARRAY_ALLOC(ioasid_xa);
>>> +static DEFINE_MUTEX(ioasid_allocator_lock);
>>> +static struct ioasid_allocator *active_custom_allocator;
>>> +
>>> +static LIST_HEAD(custom_allocators);
>>> +/*
>>> + * A flag to track if ioasid default allocator is in use, this will
>>> + * prevent custom allocator from being used. The reason is that
>>> custom allocator
>>> + * must have unadulterated space to track private data with
>>> xarray, there cannot
>>> + * be a mix been default and custom allocated IOASIDs.
>>> + */
>>> +static int default_allocator_active;
>>> +
>>> +/**
>>> + * ioasid_register_allocator - register a custom allocator
>>> + * @allocator: the custom allocator to be registered
>>> + *
>>> + * Custom allocators take precedence over the default xarray based
>>> allocator.
>>> + * Private data associated with the ASID are managed by ASID
>>> common code
>>> + * similar to data stored in xa.
>>> + *
>>> + * There can be multiple allocators registered but only one is
>>> active. In case
>>> + * of runtime removal of a custom allocator, the next one is
>>> activated based
>>> + * on the registration ordering.
>>> + */
>>> +int ioasid_register_allocator(struct ioasid_allocator *allocator)
>>> +{
>>> + struct ioasid_allocator *pallocator;
>>> + int ret = 0;
>>> +
>>> + if (!allocator)
>>> + return -EINVAL;
>> is it really necessary? Sin't it the caller responsibility?
> makes sense. will remove this one and below.
>>> +
>>> + mutex_lock(_allocator_lock);
>>> + /*
>>> +* No particular preference since all custom allocators
>>> end up calling
>>> +* the host to allocate IOASIDs. We activate the first one
>>> and keep
>>> +* the later registered allocators in a list in case the
>>> first one gets
>>> +* removed due to hotplug.
>>> +*/
>>> + if (list_empty(_allocators))
>>> + active_custom_allocator = allocator;> +
>>> else {
>>> + /* Check if the allocator is already registered */
>>> + list_for_each_entry(pallocator,
>>> _allocators, list) {
>>> + if (pallocator == allocator) {
>>> + pr_err("IOASID allocator already
>>> registered\n");
>>> + ret = -EEXIST;
>>> + goto out_unlock;
>>> + }
>>> + }
>>> + }
>>> + list_add_tail(>list, _allocators);
>>> +
>>> +out_unlock:
>>> + mutex_unlock(_allocator_lock);
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(ioasid_register_allocator);
>>> +
>>> +/**
>>> + * ioasid_unregister_allocator - Remove a custom IOASID allocator
>>> + * @allocator: the custom allocator to be removed
>>> + *
>>> + * Remove an allocator from the list, activate the next allocator
>>> in
>>> + * the order it was registered.
>>> + */
>>> +void ioasid_unregister_allocator(struct ioasid_allocator
>>> *allocator) +{
>>> + if (!allocator)
>>> + return;
>> is it really necessary?
>>> +
>>> + if (list_empty(_allocators)) {
>>> + pr_warn("No custom IOASID allocators active!\n");
>>> + return;
>>> + }
>>> +
>>> + mutex_lock(_allocator_lock);
>>> + list_del(>list);
>>> + if (list_empty(_allocators)) {
>>> + pr_info("No custom IOASID allocators\n")>
>>> + /*
>>> +* All IOASIDs should have been freed before the
>>> last custom
>>> +* allocator is unregistered. Unless default
>>> allocator is
Re: [PATCH v3 04/16] ioasid: Add custom IOASID allocator
On Tue, 21 May 2019 11:55:55 +0200
Auger Eric wrote:
> Hi Jacob,
>
> On 5/4/19 12:32 AM, Jacob Pan wrote:
> > Sometimes, IOASID allocation must be handled by platform specific
> > code. The use cases are guest vIOMMU and pvIOMMU where IOASIDs need
> > to be allocated by the host via enlightened or paravirt interfaces.
> >
> > This patch adds an extension to the IOASID allocator APIs such that
> > platform drivers can register a custom allocator, possibly at boot
> > time, to take over the allocation. Xarray is still used for tracking
> > and searching purposes internal to the IOASID code. Private data of
> > an IOASID can also be set after the allocation.
> >
> > There can be multiple custom allocators registered but only one is
> > used at a time. In case of hot removal of devices that provides the
> > allocator, all IOASIDs must be freed prior to unregistering the
> > allocator. Default XArray based allocator cannot be mixed with
> > custom allocators, i.e. custom allocators will not be used if there
> > are outstanding IOASIDs allocated by the default XA allocator.
> >
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 125
> > + 1 file changed,
> > 125 insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index 99f5e0a..ed2915a 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -17,6 +17,100 @@ struct ioasid_data {
> > };
> >
> > static DEFINE_XARRAY_ALLOC(ioasid_xa);
> > +static DEFINE_MUTEX(ioasid_allocator_lock);
> > +static struct ioasid_allocator *active_custom_allocator;
> > +
> > +static LIST_HEAD(custom_allocators);
> > +/*
> > + * A flag to track if ioasid default allocator is in use, this will
> > + * prevent custom allocator from being used. The reason is that
> > custom allocator
> > + * must have unadulterated space to track private data with
> > xarray, there cannot
> > + * be a mix been default and custom allocated IOASIDs.
> > + */
> > +static int default_allocator_active;
> > +
> > +/**
> > + * ioasid_register_allocator - register a custom allocator
> > + * @allocator: the custom allocator to be registered
> > + *
> > + * Custom allocators take precedence over the default xarray based
> > allocator.
> > + * Private data associated with the ASID are managed by ASID
> > common code
> > + * similar to data stored in xa.
> > + *
> > + * There can be multiple allocators registered but only one is
> > active. In case
> > + * of runtime removal of a custom allocator, the next one is
> > activated based
> > + * on the registration ordering.
> > + */
> > +int ioasid_register_allocator(struct ioasid_allocator *allocator)
> > +{
> > + struct ioasid_allocator *pallocator;
> > + int ret = 0;
> > +
> > + if (!allocator)
> > + return -EINVAL;
> is it really necessary? Sin't it the caller responsibility?
makes sense. will remove this one and below.
> > +
> > + mutex_lock(_allocator_lock);
> > + /*
> > +* No particular preference since all custom allocators
> > end up calling
> > +* the host to allocate IOASIDs. We activate the first one
> > and keep
> > +* the later registered allocators in a list in case the
> > first one gets
> > +* removed due to hotplug.
> > +*/
> > + if (list_empty(_allocators))
> > + active_custom_allocator = allocator;> +
> > else {
> > + /* Check if the allocator is already registered */
> > + list_for_each_entry(pallocator,
> > _allocators, list) {
> > + if (pallocator == allocator) {
> > + pr_err("IOASID allocator already
> > registered\n");
> > + ret = -EEXIST;
> > + goto out_unlock;
> > + }
> > + }
> > + }
> > + list_add_tail(>list, _allocators);
> > +
> > +out_unlock:
> > + mutex_unlock(_allocator_lock);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_register_allocator);
> > +
> > +/**
> > + * ioasid_unregister_allocator - Remove a custom IOASID allocator
> > + * @allocator: the custom allocator to be removed
> > + *
> > + * Remove an allocator from the list, activate the next allocator
> > in
> > + * the order it was registered.
> > + */
> > +void ioasid_unregister_allocator(struct ioasid_allocator
> > *allocator) +{
> > + if (!allocator)
> > + return;
> is it really necessary?
> > +
> > + if (list_empty(_allocators)) {
> > + pr_warn("No custom IOASID allocators active!\n");
> > + return;
> > + }
> > +
> > + mutex_lock(_allocator_lock);
> > + list_del(>list);
> > + if (list_empty(_allocators)) {
> > + pr_info("No custom IOASID allocators\n")>
> > + /*
> > +* All IOASIDs should have been freed before the
> > last custom
> > +* allocator is unregistered. Unless default
> > allocator is in
> > +* use.
> > +*/
> > +
Re: [PATCH v3 04/16] ioasid: Add custom IOASID allocator
Hi Jacob,
On 5/4/19 12:32 AM, Jacob Pan wrote:
> Sometimes, IOASID allocation must be handled by platform specific
> code. The use cases are guest vIOMMU and pvIOMMU where IOASIDs need
> to be allocated by the host via enlightened or paravirt interfaces.
>
> This patch adds an extension to the IOASID allocator APIs such that
> platform drivers can register a custom allocator, possibly at boot
> time, to take over the allocation. Xarray is still used for tracking
> and searching purposes internal to the IOASID code. Private data of
> an IOASID can also be set after the allocation.
>
> There can be multiple custom allocators registered but only one is
> used at a time. In case of hot removal of devices that provides the
> allocator, all IOASIDs must be freed prior to unregistering the
> allocator. Default XArray based allocator cannot be mixed with
> custom allocators, i.e. custom allocators will not be used if there
> are outstanding IOASIDs allocated by the default XA allocator.
>
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/ioasid.c | 125
> +
> 1 file changed, 125 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index 99f5e0a..ed2915a 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -17,6 +17,100 @@ struct ioasid_data {
> };
>
> static DEFINE_XARRAY_ALLOC(ioasid_xa);
> +static DEFINE_MUTEX(ioasid_allocator_lock);
> +static struct ioasid_allocator *active_custom_allocator;
> +
> +static LIST_HEAD(custom_allocators);
> +/*
> + * A flag to track if ioasid default allocator is in use, this will
> + * prevent custom allocator from being used. The reason is that custom
> allocator
> + * must have unadulterated space to track private data with xarray, there
> cannot
> + * be a mix been default and custom allocated IOASIDs.
> + */
> +static int default_allocator_active;
> +
> +/**
> + * ioasid_register_allocator - register a custom allocator
> + * @allocator: the custom allocator to be registered
> + *
> + * Custom allocators take precedence over the default xarray based allocator.
> + * Private data associated with the ASID are managed by ASID common code
> + * similar to data stored in xa.
> + *
> + * There can be multiple allocators registered but only one is active. In
> case
> + * of runtime removal of a custom allocator, the next one is activated based
> + * on the registration ordering.
> + */
> +int ioasid_register_allocator(struct ioasid_allocator *allocator)
> +{
> + struct ioasid_allocator *pallocator;
> + int ret = 0;
> +
> + if (!allocator)
> + return -EINVAL;
is it really necessary? Sin't it the caller responsibility?
> +
> + mutex_lock(_allocator_lock);
> + /*
> + * No particular preference since all custom allocators end up calling
> + * the host to allocate IOASIDs. We activate the first one and keep
> + * the later registered allocators in a list in case the first one gets
> + * removed due to hotplug.
> + */
> + if (list_empty(_allocators))
> + active_custom_allocator = allocator;> + else {
> + /* Check if the allocator is already registered */
> + list_for_each_entry(pallocator, _allocators, list) {
> + if (pallocator == allocator) {
> + pr_err("IOASID allocator already registered\n");
> + ret = -EEXIST;
> + goto out_unlock;
> + }
> + }
> + }
> + list_add_tail(>list, _allocators);
> +
> +out_unlock:
> + mutex_unlock(_allocator_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_register_allocator);
> +
> +/**
> + * ioasid_unregister_allocator - Remove a custom IOASID allocator
> + * @allocator: the custom allocator to be removed
> + *
> + * Remove an allocator from the list, activate the next allocator in
> + * the order it was registered.
> + */
> +void ioasid_unregister_allocator(struct ioasid_allocator *allocator)
> +{
> + if (!allocator)
> + return;
is it really necessary?
> +
> + if (list_empty(_allocators)) {
> + pr_warn("No custom IOASID allocators active!\n");
> + return;
> + }
> +
> + mutex_lock(_allocator_lock);
> + list_del(>list);
> + if (list_empty(_allocators)) {
> + pr_info("No custom IOASID allocators\n")> + /*
> + * All IOASIDs should have been freed before the last custom
> + * allocator is unregistered. Unless default allocator is in
> + * use.
> + */
> + BUG_ON(!xa_empty(_xa) && !default_allocator_active);
> + active_custom_allocator = NULL;
> + } else if (allocator == active_custom_allocator) {
In case you are removing the active custom allocator don't you also need
to check that all ioasids were freed. Otherwise you are likely to switch
to
