Re: [PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Oct 13, 2023 at 03:16:51PM +0000, Vasant Hegde wrote:
> -static void free_gcr3_table(struct protection_domain *domain)
> +static void free_gcr3_table(struct iommu_dev_data *dev_data)
>  {
> -	if (domain->glx == 2)
> -		free_gcr3_tbl_level2(domain->gcr3_tbl);
> -	else if (domain->glx == 1)
> -		free_gcr3_tbl_level1(domain->gcr3_tbl);
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
> +
> +	if (gcr3_info->glx == 2)
> +		free_gcr3_tbl_level2(gcr3_info->gcr3_tbl);
> +	else if (gcr3_info->glx == 1)
> +		free_gcr3_tbl_level1(gcr3_info->gcr3_tbl);
>  	else
> -		BUG_ON(domain->glx != 0);
> +		WARN_ON_ONCE(gcr3_info->glx != 0);
> +
> +	gcr3_info->glx = 0;
> +
> +	set_dte_entry(iommu, dev_data);
> +	clone_aliases(iommu, dev_data->dev);
> +	device_flush_dte(dev_data);

This looks super wonky. We free some of the gcr3 memory and *then*
update the DTE? Is that ordered properly?

As per my prior email the caller should have already updated the DTE
because it attached some non-gcr3 needing thing. Once the new DTE is
in place then free_gcr3_table() should simply just free the allocated
memory.

Commingling DTE manipulation inside the gcr3 layer is not good layering.

You should be moving to a direction where the ops->attach_dev does
exactly one update to the DTE. It loads the new correct value of the
DTE that attach_dev is asking to create. All this repeated touching of
the DTE during the attach_dev flow is sort of a functional bug, or at
least a sub-optimal implementation of the API.

> -/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
> -static int setup_gcr3_table(struct protection_domain *domain, int pasids)
> +static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
>  {
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>  	int levels = get_gcr3_levels(pasids);
>  
>  	if (levels > amd_iommu_max_glx_val)
>  		return -EINVAL;
>  
> -	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
> -	if (domain->gcr3_tbl == NULL)
> +	if (gcr3_info->gcr3_tbl)
> +		return -EBUSY;
> +
> +	gcr3_info->gcr3_tbl = alloc_pgtable_page(dev_to_node(dev_data->dev),
> +						 GFP_ATOMIC);
> +	if (gcr3_info->gcr3_tbl == NULL)
>  		return -ENOMEM;

Why is this in an atomic context? I can understand that the domain was
because it used a spinlock, but this should now be locked by the group
mutex which is not atomic.

Jason