On Mon, Nov 14, 2022 at 05:50:50AM +0000, Tian, Kevin wrote:
From: Jason Gunthorpe jgg@nvidia.com Sent: Tuesday, November 8, 2022 8:49 AM
@@ -171,7 +183,7 @@ static struct iopt_area *iopt_pages_find_domain_area(struct iopt_pages *pages, */ struct pfn_batch { unsigned long *pfns;
- u16 *npfns;
- u32 *npfns;
why not making it u32 and removing later FIXME directly in patch7?
Rebase error, I fixed it
static bool batch_add_pfn(struct pfn_batch *batch, unsigned long pfn) {
- /* FIXME: U16 is too small */
- const unsigned int MAX_NPFNS = type_max(typeof(*batch->npfns));
use lowercase i.e. max_npfns.
It is customary to use caps for constants, eg enum values and things
+static void __iopt_area_unfill_domain(struct iopt_area *area,
struct iopt_pages *pages,
struct iommu_domain *domain,
unsigned long last_index)
+{
- struct interval_tree_double_span_iter span;
- unsigned long start_index = iopt_area_index(area);
- unsigned long unmapped_end_index = start_index;
- u64 backup[BATCH_BACKUP_SIZE];
- struct pfn_batch batch;
- lockdep_assert_held(&pages->mutex);
- batch_init_backup(&batch, last_index + 1, backup, sizeof(backup));
- interval_tree_for_each_double_span(&span, &pages-
domains_itree,
&pages->access_itree, start_index,
last_index) {
if (span.is_used) {
batch_skip_carry(&batch,
span.last_used - span.start_used + 1);
continue;
}
iopt_area_unpin_domain(&batch, area, pages, domain,
span.start_hole, span.last_hole,
&unmapped_end_index, last_index);
- }
- if (unmapped_end_index != last_index + 1)
iopt_area_unmap_domain_range(area, domain,
unmapped_end_index,
last_index);
a comment marking that it's for the last trailing used span of which the pages are not contiguous to previous span.
/* * If the range ends in a access then we do the residual unmap without * any unpins. */
btw it is not easy to understand how this func plus unpin_domain() actually work. more comments are welcomed to help readability.
/* * For security we must not unpin something that is still DMA mapped, * so this must unmap any IOVA before we go ahead and unpin the pages. * This creates a complexity where we need to skip over unpinning pages * held in the xarray, but continue to unmap from the domain. * * The domain unmap cannot stop in the middle of a contiguous range of * PFNs. To solve this problem the unpinning step will read ahead to the * end of any contiguous span, unmap that whole span, and then only * unpin the leading part that does not have any accesses. The residual * PFNs that were unmapped but not unpinned are called a "carry" in the * batch as they are moved to the front of the PFN list and continue on * to the next iteration(s). */
+/*
- This can do everything and is fully coherent with what a iommu_domain
would
- see.
- */
+static int iopt_pages_rw_slow(struct iopt_pages *pages,
Can you elaborate what guarantees coherency in this function and how it becomes different in other rw variations?
/* * This uses the pfn_reader instead of taking a shortcut by using the mm. It can * do every scenario and is fully consistent with what an iommu_domain would * see. */
- iopt_pages_remove_access() - Release an in-kernel access for PFNs
- @area: The source of PFNs
- @start_index: First page index
- @last_index: Inclusive last page index
- Undo iopt_pages_add_access() and unpin the pages if necessary. The
caller
- must stop using the PFNs before calling this.
- */
+void iopt_pages_remove_access(struct iopt_area *area, unsigned long start_index,
unsigned long last_index)
this is called iopt_pages_xxx() but the first parameter is iopt_area.
also it's not balanced with iopt_pages_add_access() which requires the caller to hold pages->mutex and populate area->num_accesses.
OK, see below
Thanks, Jason
--- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -565,7 +565,7 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) - iopt_pages_remove_access( + iopt_area_remove_access( area, iopt_area_iova_to_index(area, iter.cur_iova), iopt_area_iova_to_index( area, @@ -650,15 +650,10 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, goto err_remove; }
- mutex_lock(&area->pages->mutex); - rc = iopt_pages_add_access(area->pages, index, last_index, - out_pages, flags); - if (rc) { - mutex_unlock(&area->pages->mutex); + rc = iopt_pages_add_access(area, index, last_index, out_pages, + flags); + if (rc) goto err_remove; - } - area->num_accesses++; - mutex_unlock(&area->pages->mutex); out_pages += last_index - index + 1; } if (!iopt_area_contig_done(&iter)) { @@ -673,7 +668,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, if (iova < iter.cur_iova) { last_iova = iter.cur_iova - 1; iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) - iopt_pages_remove_access( + iopt_area_remove_access( area, iopt_area_iova_to_index(area, iter.cur_iova), iopt_area_iova_to_index( diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h index 3b85fa344f6be3..68bc3957534dd7 100644 --- a/drivers/iommu/iommufd/io_pagetable.h +++ b/drivers/iommu/iommufd/io_pagetable.h @@ -221,10 +221,10 @@ int iopt_pages_fill_xarray(struct iopt_pages *pages, unsigned long start, void iopt_pages_unfill_xarray(struct iopt_pages *pages, unsigned long start, unsigned long last);
-int iopt_pages_add_access(struct iopt_pages *pages, unsigned long start, - unsigned long last, struct page **out_pages, - unsigned int flags); -void iopt_pages_remove_access(struct iopt_area *area, unsigned long start, +int iopt_area_add_access(struct iopt_area *area, unsigned long start, + unsigned long last, struct page **out_pages, + unsigned int flags); +void iopt_area_remove_access(struct iopt_area *area, unsigned long start, unsigned long last); int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte, void *data, unsigned long length, unsigned int flags); diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c index 01d2447eac4ede..e5f267d9e2b491 100644 --- a/drivers/iommu/iommufd/pages.c +++ b/drivers/iommu/iommufd/pages.c @@ -1807,8 +1826,8 @@ iopt_pages_get_exact_access(struct iopt_pages *pages, unsigned long index, }
/** - * iopt_pages_add_access() - Record an in-knerel access for PFNs - * @pages: The source of PFNs + * iopt_area_add_access() - Record an in-knerel access for PFNs + * @area: The source of PFNs * @start_index: First page index * @last_index: Inclusive last page index * @out_pages: Output list of struct page's representing the PFNs @@ -1819,40 +1838,49 @@ iopt_pages_get_exact_access(struct iopt_pages *pages, unsigned long index, * * This should be undone through a matching call to iopt_pages_remove_access() */ -int iopt_pages_add_access(struct iopt_pages *pages, unsigned long start_index, +int iopt_area_add_access(struct iopt_area *area, unsigned long start_index, unsigned long last_index, struct page **out_pages, unsigned int flags) { + struct iopt_pages *pages = area->pages; struct iopt_pages_access *access; int rc;
if ((flags & IOMMUFD_ACCESS_RW_WRITE) && !pages->writable) return -EPERM;
+ mutex_lock(&pages->mutex); access = iopt_pages_get_exact_access(pages, start_index, last_index); if (access) { refcount_inc(&access->refcount); iopt_pages_fill_from_xarray(pages, start_index, last_index, out_pages); + mutex_unlock(&pages->mutex); return 0; }
access = kzalloc(sizeof(*access), GFP_KERNEL_ACCOUNT); - if (!access) - return -ENOMEM; + if (!access) { + rc = -ENOMEM; + goto err_unlock; + }
rc = iopt_pages_fill_xarray(pages, start_index, last_index, out_pages); if (rc) - goto out_free; + goto err_free;
access->node.start = start_index; access->node.last = last_index; refcount_set(&access->refcount, 1); + area->num_accesses++; interval_tree_insert(&access->node, &pages->access_itree); + mutex_unlock(&pages->mutex); return 0;
-out_free: +err_free: kfree(access); +err_unlock: + mutex_unlock(&pages->mutex); return rc; }
@@ -1865,11 +1893,11 @@ int iopt_pages_add_access(struct iopt_pages *pages, unsigned long start_index, * Undo iopt_pages_add_access() and unpin the pages if necessary. The caller * must stop using the PFNs before calling this. */ -void iopt_pages_remove_access(struct iopt_area *area, unsigned long start_index, - unsigned long last_index) +void iopt_area_remove_access(struct iopt_area *area, unsigned long start_index, + unsigned long last_index) { - struct iopt_pages_access *access; struct iopt_pages *pages = area->pages; + struct iopt_pages_access *access;
mutex_lock(&pages->mutex); access = iopt_pages_get_exact_access(pages, start_index, last_index);