IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: "Sironi, Filippo via iommu" <iommu@lists.linux-foundation.org>
To: Joerg Roedel <joro@8bytes.org>
Cc: "iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"jroedel@suse.de" <jroedel@suse.de>
Subject: Re: [PATCH 1/6] iommu/amd: Remove domain->updated
Date: Wed, 25 Sep 2019 15:45:14 +0000
Message-ID: <7E6C7B56-442F-408E-B127-FE5CBE58A3F5@amazon.de> (raw)
In-Reply-To: <20190925132300.3038-2-joro@8bytes.org>



> On 25. Sep 2019, at 06:22, Joerg Roedel <joro@8bytes.org> wrote:
> 
> From: Joerg Roedel <jroedel@suse.de>
> 
> This struct member was used to track whether a domain
> change requires updates to the device-table and IOMMU cache
> flushes. The problem is, that access to this field is racy
> since locking in the common mapping code-paths has been
> eliminated.
> 
> Move the updated field to the stack to get rid of all
> potential races and remove the field from the struct.
> 
> Fixes: 92d420ec028d ("iommu/amd: Relax locking in dma_ops path")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
> drivers/iommu/amd_iommu.c       | 49 +++++++++++++++++----------------
> drivers/iommu/amd_iommu_types.h |  1 -
> 2 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 7bdce3b10f3d..042854bbc5bc 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1458,10 +1458,11 @@ static void free_pagetable(struct protection_domain *domain)
>  * another level increases the size of the address space by 9 bits to a size up
>  * to 64 bits.
>  */
> -static void increase_address_space(struct protection_domain *domain,
> +static bool increase_address_space(struct protection_domain *domain,
> 				   gfp_t gfp)
> {
> 	unsigned long flags;
> +	bool ret = false;
> 	u64 *pte;
> 
> 	spin_lock_irqsave(&domain->lock, flags);
> @@ -1478,19 +1479,21 @@ static void increase_address_space(struct protection_domain *domain,
> 					iommu_virt_to_phys(domain->pt_root));
> 	domain->pt_root  = pte;
> 	domain->mode    += 1;
> -	domain->updated  = true;
> +
> +	ret = true;
> 
> out:
> 	spin_unlock_irqrestore(&domain->lock, flags);
> 
> -	return;
> +	return ret;
> }
> 
> static u64 *alloc_pte(struct protection_domain *domain,
> 		      unsigned long address,
> 		      unsigned long page_size,
> 		      u64 **pte_page,
> -		      gfp_t gfp)
> +		      gfp_t gfp,
> +		      bool *updated)
> {
> 	int level, end_lvl;
> 	u64 *pte, *page;
> @@ -1498,7 +1501,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> 	BUG_ON(!is_power_of_2(page_size));
> 
> 	while (address > PM_LEVEL_SIZE(domain->mode))
> -		increase_address_space(domain, gfp);
> +		*updated = increase_address_space(domain, gfp) || *updated;
> 
> 	level   = domain->mode - 1;
> 	pte     = &domain->pt_root[PM_LEVEL_INDEX(level, address)];
> @@ -1530,7 +1533,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> 			for (i = 0; i < count; ++i)
> 				cmpxchg64(&lpte[i], __pte, 0ULL);
> 
> -			domain->updated = true;
> +			*updated = true;
> 			continue;
> 		}
> 
> @@ -1547,7 +1550,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> 			if (cmpxchg64(pte, __pte, __npte) != __pte)
> 				free_page((unsigned long)page);
> 			else if (IOMMU_PTE_PRESENT(__pte))
> -				domain->updated = true;
> +				*updated = true;
> 
> 			continue;
> 		}
> @@ -1656,28 +1659,29 @@ static int iommu_map_page(struct protection_domain *dom,
> 			  gfp_t gfp)
> {
> 	struct page *freelist = NULL;
> +	bool updated = false;
> 	u64 __pte, *pte;
> -	int i, count;
> +	int ret, i, count;
> 
> 	BUG_ON(!IS_ALIGNED(bus_addr, page_size));
> 	BUG_ON(!IS_ALIGNED(phys_addr, page_size));
> 
> +	ret = -EINVAL;
> 	if (!(prot & IOMMU_PROT_MASK))
> -		return -EINVAL;
> +		goto out;
> 
> 	count = PAGE_SIZE_PTE_COUNT(page_size);
> -	pte   = alloc_pte(dom, bus_addr, page_size, NULL, gfp);
> +	pte   = alloc_pte(dom, bus_addr, page_size, NULL, gfp, &updated);
> 
> -	if (!pte) {
> -		update_domain(dom);
> -		return -ENOMEM;
> -	}
> +	ret = -ENOMEM;
> +	if (!pte)
> +		goto out;
> 
> 	for (i = 0; i < count; ++i)
> 		freelist = free_clear_pte(&pte[i], pte[i], freelist);
> 
> 	if (freelist != NULL)
> -		dom->updated = true;
> +		updated = true;
> 
> 	if (count > 1) {
> 		__pte = PAGE_SIZE_PTE(__sme_set(phys_addr), page_size);
> @@ -1693,12 +1697,16 @@ static int iommu_map_page(struct protection_domain *dom,
> 	for (i = 0; i < count; ++i)
> 		pte[i] = __pte;
> 
> -	update_domain(dom);
> +	ret = 0;
> +
> +out:
> +	if (updated)
> +		update_domain(dom);
> 
> 	/* Everything flushed out, free pages now */
> 	free_page_list(freelist);
> 
> -	return 0;
> +	return ret;
> }
> 
> static unsigned long iommu_unmap_page(struct protection_domain *dom,
> @@ -2399,15 +2407,10 @@ static void update_device_table(struct protection_domain *domain)
> 
> static void update_domain(struct protection_domain *domain)
> {
> -	if (!domain->updated)
> -		return;
> -
> 	update_device_table(domain);
> 
> 	domain_flush_devices(domain);
> 	domain_flush_tlb_pde(domain);
> -
> -	domain->updated = false;
> }
> 
> static int dir2prot(enum dma_data_direction direction)
> @@ -3333,7 +3336,6 @@ void amd_iommu_domain_direct_map(struct iommu_domain *dom)
> 
> 	/* Update data structure */
> 	domain->mode    = PAGE_MODE_NONE;
> -	domain->updated = true;
> 
> 	/* Make changes visible to IOMMUs */
> 	update_domain(domain);
> @@ -3379,7 +3381,6 @@ int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
> 
> 	domain->glx      = levels;
> 	domain->flags   |= PD_IOMMUV2_MASK;
> -	domain->updated  = true;
> 
> 	update_domain(domain);
> 
> diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
> index 9ac229e92b07..0186501ab971 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -475,7 +475,6 @@ struct protection_domain {
> 	int glx;		/* Number of levels for GCR3 table */
> 	u64 *gcr3_tbl;		/* Guest CR3 table */
> 	unsigned long flags;	/* flags to find out type of domain */
> -	bool updated;		/* complete domain flush required */
> 	unsigned dev_cnt;	/* devices assigned to this domain */
> 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> };
> -- 
> 2.17.1
> 

Reviewed-by: Filippo Sironi <sironi@amazon.de>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25 13:22 [PATCH 0/6] iommu/amd: Locking Fixes Joerg Roedel
2019-09-25 13:22 ` [PATCH 1/6] iommu/amd: Remove domain->updated Joerg Roedel
2019-09-25 15:45   ` Sironi, Filippo via iommu [this message]
2019-09-26  6:18   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 2/6] iommu/amd: Remove amd_iommu_devtable_lock Joerg Roedel
2019-09-25 15:50   ` Sironi, Filippo via iommu
2019-09-25 15:52     ` Sironi, Filippo via iommu
2019-09-26  6:24   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 3/6] iommu/amd: Take domain->lock for complete attach/detach path Joerg Roedel
2019-09-25 15:53   ` Sironi, Filippo via iommu
2019-09-26  6:34   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 4/6] iommu/amd: Check for busy devices earlier in attach_device() Joerg Roedel
2019-09-25 15:55   ` Sironi, Filippo via iommu
2019-09-26  6:37   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 5/6] iommu/amd: Lock dev_data in attach/detach code paths Joerg Roedel
2019-09-25 15:56   ` Sironi, Filippo via iommu
2019-09-26  6:41   ` Jerry Snitselaar
2019-09-25 13:23 ` [PATCH 6/6] iommu/amd: Lock code paths traversing protection_domain->dev_list Joerg Roedel
2019-09-25 15:58   ` Sironi, Filippo via iommu
2019-09-26  6:48   ` Jerry Snitselaar
2019-09-26  0:25 ` [PATCH 0/6] iommu/amd: Locking Fixes Jerry Snitselaar
2019-09-26  5:46   ` Joerg Roedel

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7E6C7B56-442F-408E-B127-FE5CBE58A3F5@amazon.de \
    --to=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=jroedel@suse.de \
    --cc=sironi@amazon.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git