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

[Jeffrey Hugo]

On 2/27/2023 10:14 AM, Stanislaw Gruszka wrote:
> 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.

I clearly missed that and was relying on other documentation.  Thank you 
for pointing this out.  Will remove.

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

A single entry in the sg table is a contiguous mapping of memory.  If it 
wasn't contiguous, it would have to be broken up into multiple entries. 
  In the simple case, a driver is going to take the dma_address/len pair 
and hand that directly to the device.  Then the device is going to 
access every address in that range.

If the device can access every address from dma_address to dma_address + 
len, why can't it access a subset of that?

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

Ok.  I'll think on this.