[Intel-gfx] [RFC v2 06/12] drm/i915/svm: Device memory support
Niranjana Vishwanathapura
niranjana.vishwanathapura at intel.com
Wed Dec 18 22:15:26 UTC 2019
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
More information about the Intel-gfx
mailing list