linux-kernel - Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2023 09:28:32 -0700
From:   Jerry Snitselaar <jsnitsel@...hat.com>
To:     linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Cc:     Vasant Hegde <vasant.hegde@....com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Robin Murphy <robin.murphy@....com>,
        Will Deacon <will@...nel.org>, Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain
 allocation

On Tue, Apr 04, 2023 at 12:27:42AM -0700, Jerry Snitselaar wrote:
> With the addition of the V2 page table support, the domain page size
> bitmap needs to be set prior to iommu core setting up direct mappings
> for reserved regions. When reserved regions are mapped, if this is not
> done, it will be looking at the V1 page size bitmap when determining
> the page size to use in iommu_pgsize(). When it gets into the actual
> amd mapping code, a check of see if the page size is supported can
> fail, because at that point it is checking it against the V2 page size
> bitmap which only supports 4K, 2M, and 1G.
> 
> Add a check to __iommu_domain_alloc() to not override the
> bitmap if it was already set by the iommu ops domain_alloc() code path.
> 
> Cc: Vasant Hegde <vasant.hegde@....com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> Cc: Robin Murphy <robin.murphy@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Joerg Roedel <joro@...tes.org>
> Fixes: 4db6c41f0946 ("iommu/amd: Add support for using AMD IOMMU v2 page table for DMA-API")
> Signed-off-by: Jerry Snitselaar <jsnitsel@...hat.com>



I'm still not sure this is the best solution. Feels odd with adding a
check to core code to handle something one of the drivers has
done. Another thought was something like arm does, with amd_iommu_ops
dropping the const and setting the default page size bitmap in
iommu_init_pci(), but I think that would still require adding
something in the protection domain/init code to deal with it forcing
v1, so it would still require a check in the core code.

Would adding an op make more sense, with a generic op doing what the
core code currently does for setting the default? Or am I overthinking
this?

snits

> ---
>  drivers/iommu/amd/iommu.c | 6 ++----
>  drivers/iommu/iommu.c     | 9 +++++++--
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 5a505ba5467e..167da5b1a5e3 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1666,10 +1666,6 @@ static void do_attach(struct iommu_dev_data *dev_data,
>  	domain->dev_iommu[iommu->index] += 1;
>  	domain->dev_cnt                 += 1;
>  
> -	/* Override supported page sizes */
> -	if (domain->flags & PD_GIOV_MASK)
> -		domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
> -
>  	/* Update device table */
>  	set_dte_entry(iommu, dev_data->devid, domain,
>  		      ats, dev_data->iommu_v2);
> @@ -2048,6 +2044,8 @@ static int protection_domain_init_v2(struct protection_domain *domain)
>  
>  	domain->flags |= PD_GIOV_MASK;
>  
> +	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
> +
>  	if (domain_enable_v2(domain, 1)) {
>  		domain_id_free(domain->id);
>  		return -ENOMEM;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 10db680acaed..256a38371120 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1964,8 +1964,13 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>  		return NULL;
>  
>  	domain->type = type;
> -	/* Assume all sizes by default; the driver may override this later */
> -	domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
> +	/*
> +	 * If not already set, assume all sizes by default; the driver
> +	 * may override this later
> +	 */
> +	if (!domain->pgsize_bitmap)
> +		domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
> +
>  	if (!domain->ops)
>  		domain->ops = bus->iommu_ops->default_domain_ops;
>  
> -- 
> 2.38.1
>