Re: [PATCH kernel v8 13/31] vfio: powerpc/spapr: powerpc/iommu: Rework IOMMU ownership control

[David Gibson]

On Fri, Apr 10, 2015 at 04:30:55PM +1000, Alexey Kardashevskiy wrote:
> This replaces iommu_take_ownership()/iommu_release_ownership() calls
> with the callback calls and it is up to the platform code to call
> iommu_take_ownership()/iommu_release_ownership() if needed.

I think this commit message is out of date - I don't see any callbacks
here.

> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
> ---
>  arch/powerpc/include/asm/iommu.h    |  4 +--
>  arch/powerpc/kernel/iommu.c         | 50 ++++++++++++++++++++++++++++---------
>  drivers/vfio/vfio_iommu_spapr_tce.c |  4 +--
>  3 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index 667aa1a..b9e50d3 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -225,8 +225,8 @@ extern unsigned long iommu_clear_tce(struct iommu_table *tbl,
>  		unsigned long entry);
>  
>  extern void iommu_flush_tce(struct iommu_table *tbl);
> -extern int iommu_take_ownership(struct iommu_table *tbl);
> -extern void iommu_release_ownership(struct iommu_table *tbl);
> +extern int iommu_take_ownership(struct iommu_table_group *table_group);
> +extern void iommu_release_ownership(struct iommu_table_group *table_group);
>  
>  extern enum dma_data_direction iommu_tce_direction(unsigned long tce);
>  extern unsigned long iommu_direction_to_tce_perm(enum dma_data_direction dir);
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index fd49c8e..7d6089b 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1050,7 +1050,7 @@ int iommu_tce_build(struct iommu_table *tbl, unsigned long entry,
>  }
>  EXPORT_SYMBOL_GPL(iommu_tce_build);
>  
> -int iommu_take_ownership(struct iommu_table *tbl)
> +static int iommu_table_take_ownership(struct iommu_table *tbl)
>  {
>  	unsigned long sz = (tbl->it_size + 7) >> 3;
>  
> @@ -1064,19 +1064,36 @@ int iommu_take_ownership(struct iommu_table *tbl)
>  
>  	memset(tbl->it_map, 0xff, sz);
>  
> -	/*
> -	 * Disable iommu bypass, otherwise the user can DMA to all of
> -	 * our physical memory via the bypass window instead of just
> -	 * the pages that has been explicitly mapped into the iommu
> -	 */
> -	if (tbl->set_bypass)
> -		tbl->set_bypass(tbl, false);

The code to disable bypass is removed, and doesn't seem to be replaced
with anything.  That doesn't look safe.

> +	return 0;
> +}
> +
> +static void iommu_table_release_ownership(struct iommu_table *tbl);
> +
> +int iommu_take_ownership(struct iommu_table_group *table_group)
> +{
> +	int i, j, rc = 0;
> +
> +	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
> +		struct iommu_table *tbl = &table_group->tables[i];
> +
> +		if (!tbl->it_map)
> +			continue;
> +
> +		rc = iommu_table_take_ownership(tbl);
> +		if (rc) {
> +			for (j = 0; j < i; ++j)
> +				iommu_table_release_ownership(
> +						&table_group->tables[j]);
> +
> +			return rc;
> +		}
> +	}
>  
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iommu_take_ownership);
>  
> -void iommu_release_ownership(struct iommu_table *tbl)
> +static void iommu_table_release_ownership(struct iommu_table *tbl)
>  {
>  	unsigned long sz = (tbl->it_size + 7) >> 3;
>  
> @@ -1086,9 +1103,18 @@ void iommu_release_ownership(struct iommu_table *tbl)
>  	if (tbl->it_offset == 0)
>  		set_bit(0, tbl->it_map);
>  
> -	/* The kernel owns the device now, we can restore the iommu bypass */
> -	if (tbl->set_bypass)
> -		tbl->set_bypass(tbl, true);
> +}
> +
> +extern void iommu_release_ownership(struct iommu_table_group *table_group)
> +{
> +	int i;
> +
> +	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
> +		struct iommu_table *tbl = &table_group->tables[i];
> +
> +		if (tbl->it_map)
> +			iommu_table_release_ownership(tbl);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(iommu_release_ownership);
>  
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index d61aad2..9f38351 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -535,7 +535,7 @@ static int tce_iommu_attach_group(void *iommu_data,
>  		goto unlock_exit;
>  	}
>  
> -	ret = iommu_take_ownership(&table_group->tables[0]);
> +	ret = iommu_take_ownership(table_group);
>  	if (!ret)
>  		container->grp = iommu_group;
>  
> @@ -572,7 +572,7 @@ static void tce_iommu_detach_group(void *iommu_data,
>  	table_group = iommu_group_get_iommudata(iommu_group);
>  	BUG_ON(!table_group);
>  
> -	iommu_release_ownership(&table_group->tables[0]);
> +	iommu_release_ownership(table_group);
>  
>  unlock_exit:
>  	mutex_unlock(&container->lock);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: pgpRmQVRSsHBc.pgp
Description: PGP signature