|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/2] iommu: elide flushing for higher order map/unmap operations
>>> On 30.11.18 at 11:45, <paul.durrant@xxxxxxxxxx> wrote:
> This patch removes any implicit flushing that occurs in the implementation
> of map and unmap operations and, instead, adds explicit flushing at the
> end of the loops in the iommu_map/unmap() wrapper functions.
>
> Because VT-d currently performs two different types of flush dependent upon
> whether a PTE is being modified versus merely added (i.e. replacing a non-
> present PTE) a 'iommu_flush_type' enumeration is defined by this patch and
> the iommu_ops map method is modified to pass back the type of flush
> necessary for the PTE that has been populated. When a higher order mapping
> operation is done, the wrapper code performs the 'highest' level of flush
> required by the individual iommu_ops method calls, where a 'modified PTE'
> flush is deemed to be higher than a 'added PTE' flush.
I'm afraid such ordering properties may not generally exist. That is,
what you pass the flush handlers needs to be an OR of "added new
entries" and "modified existing entries". That's because at least in
the abstract case it may be that distinct flushes need to be issued
for both cases (i.e. potentially two of them).
> -static bool set_iommu_pte_present(unsigned long pt_mfn, unsigned long dfn,
> - unsigned long next_mfn, int pde_level,
> - bool iw, bool ir)
> +static enum iommu_flush_type set_iommu_pte_present(
> + unsigned long pt_mfn, unsigned long dfn, unsigned long next_mfn,
> + int pde_level, bool iw, bool ir)
> {
> uint64_t *table;
> uint32_t *pde;
> - bool need_flush;
> + enum iommu_flush_type flush_type;
>
> table = map_domain_page(_mfn(pt_mfn));
>
> pde = (uint32_t *)(table + pfn_to_pde_idx(dfn, pde_level));
>
> - need_flush = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> + flush_type = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> unmap_domain_page(table);
> - return need_flush;
> + return flush_type;
> }
Please take the opportunity and add the missing blank line.
> @@ -629,8 +629,6 @@ int amd_iommu_unmap_page(struct domain *d, dfn_t dfn)
> clear_iommu_pte_present(pt_mfn[1], dfn_x(dfn));
>
> spin_unlock(&hd->arch.mapping_lock);
> -
> - amd_iommu_flush_pages(d, dfn_x(dfn), 0);
> return 0;
> }
Please retain the blank line.
> @@ -700,12 +705,23 @@ int amd_iommu_reserve_domain_unity_map(struct domain
> *domain,
> for ( i = 0; i < npages; i++ )
> {
> unsigned long frame = gfn + i;
> + enum iommu_flush_type this_flush_type;
>
> - rt = amd_iommu_map_page(domain, _dfn(frame), _mfn(frame), flags);
> + rt = amd_iommu_map_page(domain, _dfn(frame), _mfn(frame), flags,
> + &this_flush_type);
> if ( rt != 0 )
> - return rt;
> + break;
> +
> + flush_type = MAX(flush_type, this_flush_type);
> }
> - return 0;
> +
> + /*
> + * The underlying implementation is void so the return value is
> + * meaningless and can hence be ignored.
> + */
> + ignored = amd_iommu_flush_iotlb_pages(domain, _dfn(gfn), npages,
> + flush_type);
I'm afraid such an assignment without subsequent use can be
(legitimately) warned about by compilers. Hence the approach
I had asked you to restore in one of your earlier patches. The
exact same one won't fit here, but while ( ... ) break; should.
Same then elsewhere.
> @@ -319,11 +324,18 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
>
> for ( i = 0; i < (1ul << page_order); i++ )
> {
> + enum iommu_flush_type this_flush_type;
> + int ignore;
> +
> rc = hd->platform_ops->map_page(d, dfn_add(dfn, i),
> - mfn_add(mfn, i), flags);
> + mfn_add(mfn, i), flags,
> + &this_flush_type);
>
> if ( likely(!rc) )
> + {
> + flush_type = MAX(flush_type, this_flush_type);
With the comment above this is unlikely to stay anyway, but if it
does please use max() instead. At least I can't see why you
couldn't use the typesafe variant here.
> @@ -336,12 +348,19 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> if ( hd->platform_ops->unmap_page(d, dfn_add(dfn, i)) )
> continue;
>
> + /* Something went wrong so attempt a full flush */
> + ignore = hd->platform_ops->iotlb_flush_all(d);
> +
> if ( !is_hardware_domain(d) )
> domain_crash(d);
>
> break;
> }
>
> + if ( hd->platform_ops->iotlb_flush && !this_cpu(iommu_dont_flush_iotlb) )
> + rc = hd->platform_ops->iotlb_flush(d, dfn, (1ul << page_order),
1u only please, since the function parameter is unsigned int.
> @@ -378,6 +397,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned
> int page_order)
> }
> }
>
> + if ( hd->platform_ops->iotlb_flush )
> + rc = hd->platform_ops->iotlb_flush(d, dfn, (1ul << page_order),
Same here.
> @@ -417,7 +440,9 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn,
> unsigned int page_count)
> if ( !iommu_enabled || !hd->platform_ops ||
> !hd->platform_ops->iotlb_flush
> )
> return 0;
>
> - rc = hd->platform_ops->iotlb_flush(d, dfn, page_count);
> + /* Assume a 'modified' flush is required */
> + rc = hd->platform_ops->iotlb_flush(d, dfn, page_count,
> + IOMMU_FLUSH_modified);
As per above this would then become the OR of both flush modes.
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -631,11 +631,14 @@ static int __must_check iommu_flush_iotlb(struct domain
> *d, dfn_t dfn,
> return rc;
> }
>
> -static int __must_check iommu_flush_iotlb_pages(struct domain *d,
> - dfn_t dfn,
> - unsigned int page_count)
> +static int __must_check iommu_flush_iotlb_pages(
> + struct domain *d, dfn_t dfn, unsigned int page_count,
> + enum iommu_flush_type flush_type)
Is the re-flowing really needed?
> {
> - return iommu_flush_iotlb(d, dfn, 1, page_count);
> + return (flush_type == IOMMU_FLUSH_none) ?
> + 0 :
> + iommu_flush_iotlb(d, dfn, (flush_type == IOMMU_FLUSH_modified),
Unnecessary parentheses.
> @@ -674,9 +677,6 @@ static int __must_check dma_pte_clear_one(struct domain
> *domain, u64 addr)
> spin_unlock(&hd->arch.mapping_lock);
> iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
>
> - if ( !this_cpu(iommu_dont_flush_iotlb) )
> - rc = iommu_flush_iotlb_pages(domain, daddr_to_dfn(addr), 1);
This code not getting replaced by another addition right in this
source file, and this function's only caller being
intel_iommu_unmap_page() makes me wonder why you don't
have the unmap functions similarly hand back a flush indicator.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |