On 25/11/16 15:39, David Gibson wrote:
> On Thu, Nov 24, 2016 at 04:48:08PM +1100, Alexey Kardashevskiy wrote:
>> We are going to allow the userspace to configure container in
>> one memory context and pass container fd to another so
>> we are postponing memory allocations accounted against
>> the locked memory limit. One of previous patches took care of
>> it_userspace.
>>
>> At the moment we create the default DMA window when the first group is
>> attached to a container; this is done for the userspace which is not
>> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
>> pre-registration - such client expects the default DMA window to exist.
>>
>> This postpones the default DMA window allocation till one of
>> the folliwing happens:
>> 1. first map/unmap request arrives;
>> 2. new window is requested;
>> This adds noop for the case when the userspace requested removal
>> of the default window which has not been created yet.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
>
> Hmm.. it just occurred to me: why do you even need to delay creation
> of the default window?
Because I want to account the memory it uses in locked_vm of the mm which
will later be used for map/unmap.
>
> In order to allow the open container - move to new process - map
> scenario you need to delay binding of the mm to the container until
> mapping actually occurs. And if you're doing that, you also want to
> delay allocation of the userspace table view, since the "userspace
> view" is only well defined once you're bound to a particular mm.
>
> But I don't see why you can't actually create the window - including
> the actual TCE table and all - before that point. After all, in the
> non-ddw case that's effectively what happens - the fixed window is
> there all the time.
>
> If you leave the creation of the default window at the point the first
> group is bound to the container, I think you'll simplify things -
> including because you'll remove an extra difference between the ddw
> and non-ddw cases.
>
> The you treat the binding of a table to an mm as a later step, that
> should go together with the allocation of the userspace view.
>
>> ---
>> Changes:
>> v6:
>> * new helper tce_iommu_create_default_window() moved to a separate patch;
>> * creates a default window when new window is requested; it used to
>> reset the def_window_pending flag instead;
>> * def_window_pending handling (mostly) localized in
>> tce_iommu_create_default_window() now, the only exception is removal
>> of not yet created default window.
>> ---
>> drivers/vfio/vfio_iommu_spapr_tce.c | 40 +++++++++++++++++++++++--------------
>> 1 file changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index a67bbfd..88622be 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -97,6 +97,7 @@ struct tce_container {
>> struct mutex lock;
>> bool enabled;
>> bool v2;
>> + bool def_window_pending;
>> unsigned long locked_pages;
>> struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
>> struct list_head group_list;
>> @@ -717,6 +718,9 @@ static long tce_iommu_create_default_window(struct tce_container *container)
>> struct tce_iommu_group *tcegrp;
>> struct iommu_table_group *table_group;
>>
>> + if (!container->def_window_pending)
>> + return 0;
>> +
>> if (!tce_groups_attached(container))
>> return -ENODEV;
>>
>> @@ -730,6 +734,9 @@ static long tce_iommu_create_default_window(struct tce_container *container)
>> table_group->tce32_size, 1, &start_addr);
>> WARN_ON_ONCE(!ret && start_addr);
>>
>> + if (!ret)
>> + container->def_window_pending = false;
>> +
>> return ret;
>> }
>>
>> @@ -823,6 +830,10 @@ static long tce_iommu_ioctl(void *iommu_data,
>> VFIO_DMA_MAP_FLAG_WRITE))
>> return -EINVAL;
>>
>> + ret = tce_iommu_create_default_window(container);
>> + if (ret)
>> + return ret;
>> +
>> num = tce_iommu_find_table(container, param.iova, &tbl);
>> if (num < 0)
>> return -ENXIO;
>> @@ -886,6 +897,10 @@ static long tce_iommu_ioctl(void *iommu_data,
>> if (param.flags)
>> return -EINVAL;
>>
>> + ret = tce_iommu_create_default_window(container);
>> + if (ret)
>> + return ret;
>> +
>> num = tce_iommu_find_table(container, param.iova, &tbl);
>> if (num < 0)
>> return -ENXIO;
>> @@ -1012,6 +1027,10 @@ static long tce_iommu_ioctl(void *iommu_data,
>>
>> mutex_lock(&container->lock);
>>
>> + ret = tce_iommu_create_default_window(container);
>> + if (ret)
>> + return ret;
>> +
>> ret = tce_iommu_create_window(container, create.page_shift,
>> create.window_size, create.levels,
>> &create.start_addr);
>> @@ -1044,6 +1063,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>> if (remove.flags)
>> return -EINVAL;
>>
>> + if (container->def_window_pending && !remove.start_addr) {
>> + container->def_window_pending = false;
>> + return 0;
>> + }
>> +
>> mutex_lock(&container->lock);
>>
>> ret = tce_iommu_remove_window(container, remove.start_addr);
>> @@ -1141,7 +1165,6 @@ static int tce_iommu_attach_group(void *iommu_data,
>> struct tce_container *container = iommu_data;
>> struct iommu_table_group *table_group;
>> struct tce_iommu_group *tcegrp = NULL;
>> - bool create_default_window = false;
>>
>> mutex_lock(&container->lock);
>>
>> @@ -1189,25 +1212,12 @@ static int tce_iommu_attach_group(void *iommu_data,
>> } else {
>> ret = tce_iommu_take_ownership_ddw(container, table_group);
>> if (!tce_groups_attached(container) && !container->tables[0])
>> - create_default_window = true;
>> + container->def_window_pending = true;
>> }
>>
>> if (!ret) {
>> tcegrp->grp = iommu_group;
>> list_add(&tcegrp->next, &container->group_list);
>> - /*
>> - * If it the first group attached, check if there is
>> - * a default DMA window and create one if none as
>> - * the userspace expects it to exist.
>> - */
>> - if (create_default_window) {
>> - ret = tce_iommu_create_default_window(container);
>> - if (ret) {
>> - list_del(&tcegrp->next);
>> - tce_iommu_release_ownership_ddw(container,
>> - table_group);
>> - }
>> - }
>> }
>>
>> unlock_exit:
>
--
Alexey
Attachment:
signature.asc
Description: OpenPGP digital signature
