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
