Re: [PATCH v2 5/8] accel/qaic: Add datapath

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Fri, Feb 24, 2023 at 12:36:51PM -0700, Jeffrey Hugo wrote:
> > > +static int reserve_pages(unsigned long start_pfn, unsigned long nr_pages,
> > > +			 bool reserve)
> > > +{
> > > +	unsigned long pfn;
> > > +	unsigned long end_pfn = start_pfn + nr_pages;
> > > +	struct page *page;
> > > +
> > > +	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> > > +		if (!pfn_valid(pfn))
> > > +			return -EINVAL;
> > > +		page =  pfn_to_page(pfn);
> > > +		if (reserve)
> > > +			SetPageReserved(page);
> > > +		else
> > > +			ClearPageReserved(page);
> > 
> > It is needed? Looks like taken from some legacy code.
> 
> Required for remap_pfn_range().

PG_reserved is not required any longer for remap_pfn_range(), here
is excerpt from comment from include/linux/page-flags.h :

 * Some PG_reserved pages will be excluded from the hibernation image.
 * PG_reserved does in general not hinder anybody from dumping or swapping
 * and is no longer required for remap_pfn_range(). ioremap might require it.
 * Consequently, PG_reserved for a page mapped into user space can indicate
 * the zero page, the vDSO, MMIO pages or device memory.

> > > +static int copy_sgt(struct qaic_device *qdev, struct sg_table **sgt_out,
> > > +		    struct sg_table *sgt_in, u64 size, u64 offset)
> > > +{
> > > +	int total_len, len, nents, offf = 0, offl = 0;
> > > +	struct scatterlist *sg, *sgn, *sgf, *sgl;
> > > +	struct sg_table *sgt;
> > > +	int ret, j;
> > > +
> > > +	/* find out number of relevant nents needed for this mem */
> > > +	total_len = 0;
> > > +	sgf = NULL;
> > > +	sgl = NULL;
> > > +	nents = 0;
> > > +
> > > +	size = size ? size : PAGE_SIZE;
> > > +	for (sg = sgt_in->sgl; sg; sg = sg_next(sg)) {
> > > +		len = sg_dma_len(sg);
> > > +
> > > +		if (!len)
> > > +			continue;
> > > +		if (offset >= total_len && offset < total_len + len) {
> > > +			sgf = sg;
> > > +			offf = offset - total_len;
> > > +		}
> > > +		if (sgf)
> > > +			nents++;
> > > +		if (offset + size >= total_len &&
> > > +		    offset + size <= total_len + len) {
> > > +			sgl = sg;
> > > +			offl = offset + size - total_len;
> > > +			break;
> > > +		}
> > > +		total_len += len;
> > > +	}
> > > +
> > > +	if (!sgf || !sgl) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > > +	if (!sgt) {
> > > +		ret = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
> > > +	if (ret)
> > > +		goto free_sgt;
> > > +
> > > +	/* copy relevant sg node and fix page and length */
> > > +	sgn = sgf;
> > > +	for_each_sgtable_sg(sgt, sg, j) {
> > > +		memcpy(sg, sgn, sizeof(*sg));
> > > +		if (sgn == sgf) {
> > > +			sg_dma_address(sg) += offf;

This looks a bit suspicious. Are you sure you can modify
sg->dma_address and still use it as valid value ?

> > > +			sg_dma_len(sg) -= offf;
> > > +			sg_set_page(sg, sg_page(sgn),
> > > +				    sg_dma_len(sg), offf);
> > > +		} else {
> > > +			offf = 0;
> > > +		}
> > > +		if (sgn == sgl) {
> > > +			sg_dma_len(sg) = offl - offf;
> > > +			sg_set_page(sg, sg_page(sgn),
> > > +				    offl - offf, offf);
> > > +			sg_mark_end(sg);
> > > +			break;
> > > +		}
> > > +		sgn = sg_next(sgn);
> > > +	}
> > 
> > Why not use sg_copy_table() ? Overall copy_sgt() seems to be something
> > that could be replaced by generic helper or at least simplify.
> 
> I don't see "sg_copy_table" defined in 6.2. 

Because there is no such function in any kernel source. It was only my
imagination, not sure right now how I came up with this function name :-/
Sorry about confusion.

There are only sg_copy_{to,from}_buffer(), but not really useful in
this case.

> Are you suggesting renaming
> this function?  I guess I'm not quite understanding your comment here. Can
> you elaborate?

Renaming would be nice. I was thinking by simplifying it, not sure
now if that's easy achievable, though.

Regards
Stanislaw