> On 11. Sep 2019, at 13:34, Joerg Roedel <j...@8bytes.org> wrote:
>
> Hi Filippo,
>
> On Tue, Sep 10, 2019 at 07:49:20PM +0200, Filippo Sironi wrote:
>> This patch series introduce patches to take the domain lock whenever we call
>> functions that end up calling __domain_flush_pages. Holding the domain lock
>> is
>> necessary since __domain_flush_pages traverses the device list, which is
>> protected by the domain lock.
>>
>> The first patch in the series adds a completion right after an IOTLB flush in
>> attach_device.
>
> Thanks for pointing out these locking issues and your fixes. I have been
> looking into it a bit and it seems there is more problems to take care
> of.
>
> The first problem is the racy access to domain->updated, which is best
> fixed by moving that info onto the stack don't keep it in the domain
> structure.
>
> Other than that, I think your patches are kind of the big hammer
> approach to fix it. As they are, they destroy the scalability of the
> dma-api path. So we need something more fine-grained, also if we keep in
> mind that the actual cases where we need to flush something in the
> dma-api path are very rare. The default should be to not take any lock
> in that path.
>
> How does the attached patch look to you? It is completly untested but
> should give an idea of a better way to fix these locking issues.
>
> Regards,
>
> Joerg
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 61de81965c44..bb93a2bbb73d 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1435,9 +1435,10 @@ 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)
> {
> + bool updated = false;
> unsigned long flags;
> u64 *pte;
>
> @@ -1455,27 +1456,30 @@ 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;
> + updated = true;
>
> out:
> spin_unlock_irqrestore(&domain->lock, flags);
>
> - return;
> + return updated;
> }
>
> 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;
>
> BUG_ON(!is_power_of_2(page_size));
>
> + *updated = false;
> +
> 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)];
> @@ -1501,7 +1505,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> if (cmpxchg64(pte, __pte, __npte) != __pte)
> free_page((unsigned long)page);
> else if (pte_level == PAGE_MODE_7_LEVEL)
> - domain->updated = true;
> + *updated = true;
>
> continue;
> }
> @@ -1617,6 +1621,7 @@ static int iommu_map_page(struct protection_domain *dom,
> struct page *freelist = NULL;
> u64 __pte, *pte;
> int i, count;
> + bool updated;
>
> BUG_ON(!IS_ALIGNED(bus_addr, page_size));
> BUG_ON(!IS_ALIGNED(phys_addr, page_size));
> @@ -1625,7 +1630,7 @@ static int iommu_map_page(struct protection_domain *dom,
> return -EINVAL;
>
> 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)
> return -ENOMEM;
> @@ -1634,7 +1639,7 @@ static int iommu_map_page(struct protection_domain *dom,
> 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);
> @@ -1650,7 +1655,8 @@ static int iommu_map_page(struct protection_domain *dom,
> for (i = 0; i < count; ++i)
> pte[i] = __pte;
>
> - update_domain(dom);
> + if (updated)
> + update_domain(dom);
>
> /* Everything flushed out, free pages now */
> free_page_list(freelist);
> @@ -2041,6 +2047,13 @@ static int __attach_device(struct iommu_dev_data
> *dev_data,
> /* Attach alias group root */
> do_attach(dev_data, domain);
>
> + /*
> + * We might boot into a crash-kernel here. The crashed kernel
> + * left the caches in the IOMMU dirty. So we have to flush
> + * here to evict all dirty stuff.
> + */
> + domain_flush_tlb_pde(domain);
> +
> ret = 0;
>
> out_unlock:
> @@ -2162,13 +2175,6 @@ static int attach_device(struct device *dev,
> ret = __attach_device(dev_data, domain);
> spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
>
> - /*
> - * We might boot into a crash-kernel here. The crashed kernel
> - * left the caches in the IOMMU dirty. So we have to flush
> - * here to evict all dirty stuff.
> - */
> - domain_flush_tlb_pde(domain);
> -
> return ret;
> }
>
> @@ -2352,17 +2358,21 @@ static void update_device_table(struct
> protection_domain *domain)
> }
> }
>
> -static void update_domain(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 void update_domain(struct protection_domain *domain)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&domain->lock, flags);
> + __update_domain(domain);
> + spin_unlock_irqrestore(&domain->lock, flags);
> }
>
> static int dir2prot(enum dma_data_direction direction)
> @@ -3221,9 +3231,12 @@ static bool amd_iommu_is_attach_deferred(struct
> iommu_domain *domain,
> static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
> {
> struct protection_domain *dom = to_pdomain(domain);
> + unsigned long flags;
>
> + spin_lock_irqsave(&dom->lock, flags);
> domain_flush_tlb_pde(dom);
> domain_flush_complete(dom);
> + spin_unlock_irqrestore(&dom->lock, flags);
> }
>
> static void amd_iommu_iotlb_range_add(struct iommu_domain *domain,
> @@ -3285,10 +3298,9 @@ 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);
> + __update_domain(domain);
>
> /* Page-table is not visible to IOMMU anymore, so free it */
> free_pagetable(domain);
> @@ -3331,9 +3343,8 @@ 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);
> + __update_domain(domain);
>
> ret = 0;
>
> diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
> index 64edd5a9694c..143e1bf70c40 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 */
> };
Hi Joerg,
I agree with the assessment, taking the domain lock everywhere is definitely
a big hammer.
I didn't test your patch but it looks sane.
Filippo
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
