Re: [Intel-gfx] [RFC v2 06/12] drm/i915/svm: Device memory support

[Jason Gunthorpe <jgg@mellanox.com>]

On Fri, Dec 13, 2019 at 01:56:08PM -0800, Niranjana Vishwanathapura wrote:
> @@ -169,6 +170,11 @@ static int i915_range_fault(struct svm_notifier *sn,
>  			return ret;
>  		}
>  
> +		/* For dgfx, ensure the range is in device local memory only */
> +		regions = i915_dmem_convert_pfn(vm->i915, &range);
> +		if (!regions || (IS_DGFX(vm->i915) && (regions & REGION_SMEM)))
> +			return -EINVAL;
> +

This is not OK, as I said before, the driver cannot de-reference pfns
before doing the retry check, under lock.

> +
> +int i915_dmem_convert_pfn(struct drm_i915_private *dev_priv,
> +			  struct hmm_range *range)
> +{
> +	unsigned long i, npages;
> +	int regions = 0;
> +
> +	npages = (range->end - range->start) >> PAGE_SHIFT;
> +	for (i = 0; i < npages; ++i) {
> +		struct i915_buddy_block *block;
> +		struct intel_memory_region *mem;
> +		struct page *page;
> +		u64 addr;
> +
> +		page = hmm_device_entry_to_page(range, range->pfns[i]);
                        ^^^^^^^^^^^^^^^^^^^^^^

For instance, that cannot be done on a speculatively loaded page.

This also looks like it suffers from the same bug as

> +		if (!page)
> +			continue;
> +
> +		if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
> +			regions |= REGION_SMEM;
> +			continue;
> +		}
> +
> +		if (!i915_dmem_page(dev_priv, page)) {
> +			WARN(1, "Some unknown device memory !\n");

Why is that a WARN? The user could put other device memory in the
address space. You need to 'segfault' the GPU execution if this happens.

> +			range->pfns[i] = 0;
> +			continue;
> +		}
> +
> +		regions |= REGION_LMEM;
> +		block = page->zone_device_data;
> +		mem = block->private;
> +		addr = mem->region.start +
> +		       i915_buddy_block_offset(block);
> +		addr += (page_to_pfn(page) - block->pfn_first) << PAGE_SHIFT;
> +
> +		range->pfns[i] &= ~range->flags[HMM_PFN_DEVICE_PRIVATE];
> +		range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
> +		range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;

This makes more sense as a direct manipulation of the sgl, not sure
why this routine is split out from the sgl builder?

Jason
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx