On 22/11/16 13:50, David Gibson wrote:
> On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:
>> As mentioned in the previous patch, 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. The previous patch 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 first map/unmap
>> request happens.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
>> ---
>> drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
>> 1 file changed, 47 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index 442baac..1c02498 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;
>> @@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
>> WARN_ON(!ret && !(*ptbl)->it_ops->free);
>> WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
>>
>> - if (!ret && container->v2) {
>> - ret = tce_iommu_userspace_view_alloc(*ptbl);
>> - if (ret)
>> - (*ptbl)->it_ops->free(*ptbl);
>> - }
>
> Does this stuff for the user view belong in the previous patch?
Yes it does, my mistake, will fix.
>
>> -
>> - if (ret)
>> - decrement_locked_vm(table_size >> PAGE_SHIFT);
>> -
>> return ret;
>> }
>>
>> @@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
>> return 0;
>> }
>>
>> +static long tce_iommu_create_default_window(struct tce_container *container)
>> +{
>> + long ret;
>> + __u64 start_addr = 0;
>> + struct tce_iommu_group *tcegrp;
>> + struct iommu_table_group *table_group;
>> +
>> + if (!tce_groups_attached(container))
>> + return -ENODEV;
>> +
>> + tcegrp = list_first_entry(&container->group_list,
>> + struct tce_iommu_group, next);
>> + table_group = iommu_group_get_iommudata(tcegrp->grp);
>> + if (!table_group)
>> + return -ENODEV;
>> +
>> + ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
>> + table_group->tce32_size, 1, &start_addr);
>> + WARN_ON_ONCE(!ret && start_addr);
>> +
>> + return ret;
>> +}
>> +
>> static long tce_iommu_ioctl(void *iommu_data,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>> VFIO_DMA_MAP_FLAG_WRITE))
>> return -EINVAL;
>>
>> + if (container->def_window_pending) {
>> + ret = tce_iommu_create_default_window(container);
>> + if (ret)
>> + return ret;
>> + container->def_window_pending = false;
>
> Would it make sense to clear (and maybe test) def_window_pending
> within create_default_window()?
Dunno, matter of taste I suppose. I'll move it there.
>
>> + }
>> +
>> num = tce_iommu_find_table(container, param.iova, &tbl);
>> if (num < 0)
>> return -ENXIO;
>> @@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>> if (param.flags)
>> return -EINVAL;
>>
>> + if (container->def_window_pending) {
>> + ret = tce_iommu_create_default_window(container);
>> + if (ret)
>> + return ret;
>> + container->def_window_pending = false;
>> + }
>> +
>> num = tce_iommu_find_table(container, param.iova, &tbl);
>> if (num < 0)
>> return -ENXIO;
>> @@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
>>
>> mutex_lock(&container->lock);
>>
>> + container->def_window_pending = false;
>
> Uh.. why is it cleared here, without calling
> tce_iommu_create_default_window() AFAICT?
It is a branch which creates new window, if we do not have a default
window, then it will be created as the result of this ioctl(), if there is
a default window, then the flag should be false already.
>
>> ret = tce_iommu_create_window(container, create.page_shift,
>> create.window_size, create.levels,
>> &create.start_addr);
>> @@ -1030,6 +1061,12 @@ 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;
>> + }
>> + container->def_window_pending = false;
>> +
>> mutex_lock(&container->lock);
>>
>> ret = tce_iommu_remove_window(container, remove.start_addr);
>> @@ -1109,9 +1146,6 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>> static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>> struct iommu_table_group *table_group)
>> {
>> - long i, ret = 0;
>> - struct iommu_table *tbl = NULL;
>> -
>> if (!table_group->ops->create_table || !table_group->ops->set_window ||
>> !table_group->ops->release_ownership) {
>> WARN_ON_ONCE(1);
>> @@ -1120,47 +1154,9 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>>
>> table_group->ops->take_ownership(table_group);
>>
>> - /*
>> - * 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 (!tce_groups_attached(container) && !container->tables[0]) {
>> - ret = tce_iommu_create_table(container,
>> - table_group,
>> - 0, /* window number */
>> - IOMMU_PAGE_SHIFT_4K,
>> - table_group->tce32_size,
>> - 1, /* default levels */
>> - &tbl);
>> - if (ret)
>> - goto release_exit;
>> - else
>> - container->tables[0] = tbl;
>> - }
>> -
>> - /* Set all windows to the new group */
>> - for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
>> - tbl = container->tables[i];
>> -
>> - if (!tbl)
>> - continue;
>> -
>> - /* Set the default window to a new group */
>> - ret = table_group->ops->set_window(table_group, i, tbl);
>
> Uh... nothing in the new code seems to replace these set_window (and
> unset_window) calls. What's up with that?
tce_iommu_create_table() + set_window() is replaced with
tce_iommu_create_default_window() which calls tce_iommu_create_window()
which calls tce_iommu_create_table() + set_window().
I'll split this patch into 2 patches, one with the change I just explained
and one to postpone the default window creation.
>
>> - if (ret)
>> - goto release_exit;
>> - }
>> + container->def_window_pending = true;
>>
>> return 0;
>> -
>> -release_exit:
>> - for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
>> - table_group->ops->unset_window(table_group, i);
>> -
>> - table_group->ops->release_ownership(table_group);
>> -
>> - return ret;
>> }
>>
>> static int tce_iommu_attach_group(void *iommu_data,
>
--
Alexey
Attachment:
signature.asc
Description: OpenPGP digital signature
