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

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Dec 05, 2022 at 11:58:41PM +0800, Binbin Wu wrote:

> > +/*
> > + * 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?

No, in this case the caller is not allowed to incorrectly set keep_pfns.

At best we could do an assertion:

	if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
		WARN_ON(!batch->end ||
			batch->npfns[batch->end - 1] < keep_pfns);
 
> > +	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?

They are the same thing, a later patch adds an assertion to make that clear:

	if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
		WARN_ON(batch->total_pfns != batch->npfns[0]);

> > +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.

I would consider this good english, though I can see why it is not
clear.

/* true if the pfn was added, false otherwise */

> > +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?

This is correct.. However, the code was written so that we don't ever
truncate any set low bits on PHYS_PFN, which is perhaps overkill.

Given that we already must calculate page_offset I think we may as
well leave it for clarity.

> > +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?

Too late, it is already applied

I will put the comment fixes in a new commit.

Jason