[RFC v2 06/12] drm/i915/svm: Device memory support

[Niranjana Vishwanathapura]

On Tue, Dec 17, 2019 at 08:35:47PM +0000, Jason Gunthorpe wrote:
>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.
>

Thanks.
Ok, will push it down and do it after validating the range.

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

Ok.

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

OK, will return an error here if user is trying to bind here.
I agree, we need to segfault the GPU if it is GPU fault handling.

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

Ok, yah, let me merge it with sgl building.

Thanks,
Niranjana

>Jason