Re: [PATCH v6 08/19] iommufd: PFN handling for iopt_pages

[Binbin Wu <binbin.wu@linux.intel.com>]

On 11/30/2022 4:29 AM, Jason Gunthorpe wrote:
> +#ifndef __IO_PAGETABLE_H
> +#define __IO_PAGETABLE_H
> +
> +#include <linux/interval_tree.h>
> +#include <linux/mutex.h>
> +#include <linux/kref.h>
> +#include <linux/xarray.h>
> +
> +#include "iommufd_private.h"
> +
> +struct iommu_domain;
> +
> +/*
> + * Each io_pagetable is composed of intervals of areas which cover regions of
> + * the iova that are backed by something. iova not covered by areas is not
> + * populated in the page table. Each area is fully populated with pages.
> + *
> + * iovas are in byte units, but must be iopt->iova_alignment aligned.
> + *
> + * pages can be NULL, this means some other thread is still working on setting
> + * up or tearing down the area. When observed under the write side of the
> + * domain_rwsem a NULL pages must mean the area is still being setup and no
> + * domains are filled.
> + *
> + * storage_domain points at an arbitrary iommu_domain that is holding the PFNs
> + * for this area. It is locked by the pages->mutex. This simplifies the locking
> + * as the pages code can rely on the storage_domain without having to get the
> + * iopt->domains_rwsem.
> + *
> + * The io_pagetable::iova_rwsem protects node
> + * The iopt_pages::mutex protects pages_node
> + * iopt and immu_prot

typo, immu_prot -> iommu_prot


>   are immutable

> +
> +/*
> + * Carry means we carry a portion of the final hugepage over to the front of the
> + * batch
> + */
> +static void batch_clear_carry(struct pfn_batch *batch, unsigned int keep_pfns)
> +{
> +	if (!keep_pfns)
> +		return batch_clear(batch);
> +
> +	batch->total_pfns = keep_pfns;
> +	batch->npfns[0] = keep_pfns;
> +	batch->pfns[0] = batch->pfns[batch->end - 1] +
> +			 (batch->npfns[batch->end - 1] - keep_pfns);

The range of the skip_pfns is checked in batch_skip_carry, should 
keep_pfns also be checked in this function?


> +	batch->end = 0;
> +}
> +
> +static void batch_skip_carry(struct pfn_batch *batch, unsigned int skip_pfns)
> +{
> +	if (!batch->total_pfns)
> +		return;
> +	skip_pfns = min(batch->total_pfns, skip_pfns);

Should use batch->npfns[0] instead of batch->total_pfns?



> +	batch->pfns[0] += skip_pfns;
> +	batch->npfns[0] -= skip_pfns;
> +	batch->total_pfns -= skip_pfns;
> +}
> +
> +static int __batch_init(struct pfn_batch *batch, size_t max_pages, void *backup,
> +			size_t backup_len)
> +{
> +	const size_t elmsz = sizeof(*batch->pfns) + sizeof(*batch->npfns);
> +	size_t size = max_pages * elmsz;
> +
> +	batch->pfns = temp_kmalloc(&size, backup, backup_len);
> +	if (!batch->pfns)
> +		return -ENOMEM;
> +	batch->array_size = size / elmsz;
> +	batch->npfns = (u32 *)(batch->pfns + batch->array_size);
> +	batch_clear(batch);
> +	return 0;
> +}
> +
> +static int batch_init(struct pfn_batch *batch, size_t max_pages)
> +{
> +	return __batch_init(batch, max_pages, NULL, 0);
> +}
> +
> +static void batch_init_backup(struct pfn_batch *batch, size_t max_pages,
> +			      void *backup, size_t backup_len)
> +{
> +	__batch_init(batch, max_pages, backup, backup_len);
> +}
> +
> +static void batch_destroy(struct pfn_batch *batch, void *backup)
> +{
> +	if (batch->pfns != backup)
> +		kfree(batch->pfns);
> +}
> +
> +/* true if the pfn could be added, false otherwise */

It is not accurate to use "could be" here because returning ture means 
the pfn has been added.


> +static bool batch_add_pfn(struct pfn_batch *batch, unsigned long pfn)
> +{
> +	const unsigned int MAX_NPFNS = type_max(typeof(*batch->npfns));
> +
> +	if (batch->end &&
> +	    pfn == batch->pfns[batch->end - 1] + batch->npfns[batch->end - 1] &&
> +	    batch->npfns[batch->end - 1] != MAX_NPFNS) {
> +		batch->npfns[batch->end - 1]++;
> +		batch->total_pfns++;
> +		return true;
> +	}
> +	if (batch->end == batch->array_size)
> +		return false;
> +	batch->total_pfns++;
> +	batch->pfns[batch->end] = pfn;
> +	batch->npfns[batch->end] = 1;
> +	batch->end++;
> +	return true;
> +}
> +
> +/*
> + * Fill the batch with pfns from the domain. When the batch is full, or it
> + * reaches last_index, the function will return. The caller should use
> + * batch->total_pfns to determine the starting point for the next iteration.
> + */
> +static void batch_from_domain(struct pfn_batch *batch,
> +			      struct iommu_domain *domain,
> +			      struct iopt_area *area, unsigned long start_index,
> +			      unsigned long last_index)
> +{
> +	unsigned int page_offset = 0;
> +	unsigned long iova;
> +	phys_addr_t phys;
> +
> +	iova = iopt_area_index_to_iova(area, start_index);
> +	if (start_index == iopt_area_index(area))
> +		page_offset = area->page_offset;
> +	while (start_index <= last_index) {
> +		/*
> +		 * This is pretty slow, it would be nice to get the page size
> +		 * back from the driver, or have the driver directly fill the
> +		 * batch.
> +		 */
> +		phys = iommu_iova_to_phys(domain, iova) - page_offset;

seems no need to handle the page_offset, since PHYS_PFN(phys) is used in 
batch_add_pfn below?



> +		if (!batch_add_pfn(batch, PHYS_PFN(phys)))
> +			return;
> +		iova += PAGE_SIZE - page_offset;
> +		page_offset = 0;
> +		start_index++;
> +	}
> +}
> +
> +static struct page **raw_pages_from_domain(struct iommu_domain *domain,
> +					   struct iopt_area *area,
> +					   unsigned long start_index,
> +					   unsigned long last_index,
> +					   struct page **out_pages)
> +{
> +	unsigned int page_offset = 0;
> +	unsigned long iova;
> +	phys_addr_t phys;
> +
> +	iova = iopt_area_index_to_iova(area, start_index);
> +	if (start_index == iopt_area_index(area))
> +		page_offset = area->page_offset;
> +	while (start_index <= last_index) {
> +		phys = iommu_iova_to_phys(domain, iova) - page_offset;

ditto, since only PHYS_PFN(phys) is actually used, no need to handle 
page_offset



> +		*(out_pages++) = pfn_to_page(PHYS_PFN(phys));
> +		iova += PAGE_SIZE - page_offset;
> +		page_offset = 0;
> +		start_index++;
> +	}
> +	return out_pages;
> +}
> +
> +/* Continues reading a domain until we reach a discontiguity

typo, discontiguity -> discontinuity


>   in the pfns. */
> +static void batch_from_domain_continue(struct pfn_batch *batch,
> +				       struct iommu_domain *domain,
> +				       struct iopt_area *area,
> +				       unsigned long start_index,
> +				       unsigned long last_index)
> +{
> +	unsigned int array_size = batch->array_size;
> +
> +	batch->array_size = batch->end;
> +	batch_from_domain(batch, domain, area, start_index, last_index);
> +	batch->array_size = array_size;
> +}
> +

BTW, this is a quite big patch, maybe break into smaller ones?