Re: [PATCH 01/14] iommu/amd: Move allocation of the top table into v1_alloc_pgtable

[Joerg Roedel <joro@8bytes.org>]

On Wed, Aug 21, 2024 at 02:37:07PM -0300, Jason Gunthorpe wrote:
> All the page table memory should be allocated/free within the io_pgtable
> struct. The v2 path is already doing this, make it consistent.
> 
> It is hard to see but the free of the root in protection_domain_free() is
> a NOP on the success path because v1_free_pgtable() does
> amd_iommu_domain_clr_pt_root().
> 
> The root memory is already freed because free_sub_pt() put it on the
> freelist. The free path in protection_domain_free() is only used during
> error unwind of protection_domain_alloc().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/amd/io_pgtable.c |  8 ++++++--
>  drivers/iommu/amd/iommu.c      | 21 ++-------------------
>  2 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/amd/io_pgtable.c b/drivers/iommu/amd/io_pgtable.c
> index 1074ee25064d06..05aed3cb46f1bf 100644
> --- a/drivers/iommu/amd/io_pgtable.c
> +++ b/drivers/iommu/amd/io_pgtable.c
> @@ -574,20 +574,24 @@ static void v1_free_pgtable(struct io_pgtable *iop)
>  	       pgtable->mode > PAGE_MODE_6_LEVEL);
>  
>  	free_sub_pt(pgtable->root, pgtable->mode, &freelist);
> +	iommu_put_pages_list(&freelist);

You are freeing pages here before the IOMMU and device TLBs are flushed,
no?

>  
>  	/* Update data structure */
>  	amd_iommu_domain_clr_pt_root(dom);
>  
>  	/* Make changes visible to IOMMUs */
>  	amd_iommu_domain_update(dom);
> -
> -	iommu_put_pages_list(&freelist);
>  }

Regards,

	Joerg