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 <a...@ozlabs.ru>
> 
> 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

Reply via email to