From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CB913CAC5B8 for ; Thu, 2 Oct 2025 09:04:29 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 850A610E32B; Thu, 2 Oct 2025 09:04:29 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="lWM7hsRn"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.19]) by gabe.freedesktop.org (Postfix) with ESMTPS id 50C0C10E32B for ; Thu, 2 Oct 2025 09:04:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1759395868; x=1790931868; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=/u9fRnz4lM6KIO5AAj/85BeSlHr70wsHLhXrjzcqTLo=; b=lWM7hsRnZ5uJXSD5oY+rrcni2RLnDxWMdCV/7fttzA3vNyQatbLsh154 haOArVqQ/mcnJDSlXP2oTOf2rHNB7ypGY6i5511cGg6yKrH5ymoNMEMX5 cPNHWWrKIgWr11B0K1z7Ui1B4DMiMmgRqSixI2NW1N+ji2EMow6xks5VC a3Oca2LFamWNvcDCbFecjRcwm0Iqp/buJNmbN/pkwtroKV7iOyl72KxQq GcsUrF7UXtVroZO08iLSWMHQUYi0xpckRsTk/zsCvPg144jzry8AuGnrE kmKhDygg8Td9mo5SY8pLIU7AxZgSop/IcsJba5oukYpUHpxKr/vZtYpIb w==; X-CSE-ConnectionGUID: avW5+TeESGCFQ5xXcga8rA== X-CSE-MsgGUID: pGt3vbqUR9yn9RPorsaTYA== X-IronPort-AV: E=McAfee;i="6800,10657,11569"; a="60716001" X-IronPort-AV: E=Sophos;i="6.18,309,1751266800"; d="scan'208";a="60716001" Received: from orviesa005.jf.intel.com ([10.64.159.145]) by fmvoesa113.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Oct 2025 02:04:27 -0700 X-CSE-ConnectionGUID: /I/cpLUhRtOP3GicORQV/w== X-CSE-MsgGUID: ENYCJ0nNTBKFOijgNkp8xQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.18,309,1751266800"; d="scan'208";a="184195556" Received: from smoticic-mobl1.ger.corp.intel.com (HELO [10.245.245.192]) ([10.245.245.192]) by orviesa005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Oct 2025 02:04:26 -0700 Message-ID: <0542f393-3e1f-4a7d-bcb9-c3c62ff9b628@linux.intel.com> Date: Thu, 2 Oct 2025 11:04:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages To: Matthew Brost , =?UTF-8?Q?Thomas_Hellstr=C3=B6m?= Cc: Matthew Auld , intel-xe@lists.freedesktop.org, Himal Prasad Ghimiray References: <20250930072144.12170-1-thomas.hellstrom@linux.intel.com> <801fda09-2865-44fe-9a11-bb53b145f649@intel.com> <840c1e3a-bac4-4827-bcc9-c5096d07b6af@linux.intel.com> Content-Language: en-US From: Maarten Lankhorst In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" Acked-by: Maarten Lankhorst Den 2025-10-01 kl. 00:58, skrev Matthew Brost: > On Tue, Sep 30, 2025 at 02:26:00PM +0200, Thomas Hellström wrote: >> >> On 9/30/25 13:16, Matthew Auld wrote: >>> On 30/09/2025 08:21, Thomas Hellström wrote: >>>> When userptr is used on SVM-enabled VMs, a non-NULL >>>> hmm_range::dev_private_owner value might mean that >>>> hmm_range_fault() attempts to return device private pages. >>>> Either that will fail, or the userptr code will not know >>>> how to handle those. >>>> >>>> Use NULL for hmm_range::dev_private_owner to migrate >>>> such pages to system. In order to do that, move the >>>> struct drm_gpusvm::device_private_page_owner field to >>>> struct drm_gpusvm_ctx::device_private_page_owner so that >>>> it doesn't remain immutable over the drm_gpusvm lifetime. >>>> >>>> v2: >>>> - Don't conditionally compile xe_svm_devm_owner(). >>>> - Kerneldoc xe_svm_devm_owner(). >>>> >>>> Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm") >>>> Cc: Matthew Auld >>>> Cc: Himal Prasad Ghimiray >>>> Cc: Matthew Brost >>>> Signed-off-by: Thomas Hellström >>> >>> So in theory this should give the same behavior pre gpusvm conversion >>> for userptr? >> >> Actually not, the same problem is there, but we need to have a patch that >> fixes this in Linus' tree before sending a backported patch that fixes the >> behaviour before the conversion. >> >> On top of this, we will then as a follow-up actually allow VRAM pages in >> userptr with the SVM code modified to handle this, since l0 has a some >> use-cases that are not fully ported to SVM yet that would otherwise see >> multiple migration blits for the same memory. >> >>> >>> I guess missing Cc: dri-devel ? >> >> Right. I'll resend with that CC. >> >>> >>> Reviewed-by: Matthew Auld > > Reviewed-by: Matthew Brost > >> >> Thanks, >> >> Thomas >> >>> >>>> --- >>>>   drivers/gpu/drm/drm_gpusvm.c    | 24 +++++++++++++----------- >>>>   drivers/gpu/drm/xe/xe_svm.c     | 11 +++-------- >>>>   drivers/gpu/drm/xe/xe_svm.h     | 14 ++++++++++++++ >>>>   drivers/gpu/drm/xe/xe_userptr.c |  1 + >>>>   drivers/gpu/drm/xe/xe_vm.c      |  1 + >>>>   include/drm/drm_gpusvm.h        |  7 ++++--- >>>>   6 files changed, 36 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c >>>> index eeeeb99cfdf6..cb906765897e 100644 >>>> --- a/drivers/gpu/drm/drm_gpusvm.c >>>> +++ b/drivers/gpu/drm/drm_gpusvm.c >>>> @@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops >>>> drm_gpusvm_notifier_ops = { >>>>    * @name: Name of the GPU SVM. >>>>    * @drm: Pointer to the DRM device structure. >>>>    * @mm: Pointer to the mm_struct for the address space. >>>> - * @device_private_page_owner: Device private pages owner. >>>>    * @mm_start: Start address of GPU SVM. >>>>    * @mm_range: Range of the GPU SVM. >>>>    * @notifier_size: Size of individual notifiers. >>>> @@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops >>>> drm_gpusvm_notifier_ops = { >>>>    */ >>>>   int drm_gpusvm_init(struct drm_gpusvm *gpusvm, >>>>               const char *name, struct drm_device *drm, >>>> -            struct mm_struct *mm, void *device_private_page_owner, >>>> +            struct mm_struct *mm, >>>>               unsigned long mm_start, unsigned long mm_range, >>>>               unsigned long notifier_size, >>>>               const struct drm_gpusvm_ops *ops, >>>> @@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm, >>>>           mmgrab(mm); >>>>       } else { >>>>           /* No full SVM mode, only core drm_gpusvm_pages API. */ >>>> -        if (ops || num_chunks || mm_range || notifier_size || >>>> -            device_private_page_owner) >>>> +        if (ops || num_chunks || mm_range || notifier_size) >>>>               return -EINVAL; >>>>       } >>>>         gpusvm->name = name; >>>>       gpusvm->drm = drm; >>>>       gpusvm->mm = mm; >>>> -    gpusvm->device_private_page_owner = device_private_page_owner; >>>>       gpusvm->mm_start = mm_start; >>>>       gpusvm->mm_range = mm_range; >>>>       gpusvm->notifier_size = notifier_size; >>>> @@ -684,6 +681,7 @@ static unsigned int >>>> drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn, >>>>    * @notifier: Pointer to the GPU SVM notifier structure >>>>    * @start: Start address >>>>    * @end: End address >>>> + * @dev_private_owner: The device private page owner >>>>    * >>>>    * Check if pages between start and end have been faulted in on >>>> the CPU. Use to >>>>    * prevent migration of pages without CPU backing store. >>>> @@ -692,14 +690,15 @@ static unsigned int >>>> drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn, >>>>    */ >>>>   static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm, >>>>                      struct drm_gpusvm_notifier *notifier, >>>> -                   unsigned long start, unsigned long end) >>>> +                   unsigned long start, unsigned long end, >>>> +                   void *dev_private_owner) >>>>   { >>>>       struct hmm_range hmm_range = { >>>>           .default_flags = 0, >>>>           .notifier = ¬ifier->notifier, >>>>           .start = start, >>>>           .end = end, >>>> -        .dev_private_owner = gpusvm->device_private_page_owner, >>>> +        .dev_private_owner = dev_private_owner, >>>>       }; >>>>       unsigned long timeout = >>>>           jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); >>>> @@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct >>>> drm_gpusvm *gpusvm, >>>>    * @gpuva_start: Start address of GPUVA which mirrors CPU >>>>    * @gpuva_end: End address of GPUVA which mirrors CPU >>>>    * @check_pages_threshold: Check CPU pages for present threshold >>>> + * @dev_private_owner: The device private page owner >>>>    * >>>>    * This function determines the chunk size for the GPU SVM range >>>> based on the >>>>    * fault address, GPU SVM chunk sizes, existing GPU SVM ranges, >>>> and the virtual >>>> @@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm >>>> *gpusvm, >>>>                   unsigned long fault_addr, >>>>                   unsigned long gpuva_start, >>>>                   unsigned long gpuva_end, >>>> -                unsigned long check_pages_threshold) >>>> +                unsigned long check_pages_threshold, >>>> +                void *dev_private_owner) >>>>   { >>>>       unsigned long start, end; >>>>       int i = 0; >>>> @@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm >>>> *gpusvm, >>>>            * process-many-malloc' mallocs at least 64k at a time. >>>>            */ >>>>           if (end - start <= check_pages_threshold && >>>> -            !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) { >>>> +            !drm_gpusvm_check_pages(gpusvm, notifier, start, end, >>>> dev_private_owner)) { >>>>               ++i; >>>>               goto retry; >>>>           } >>>> @@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct >>>> drm_gpusvm *gpusvm, >>>>       chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas, >>>>                            fault_addr, gpuva_start, >>>>                            gpuva_end, >>>> -                         ctx->check_pages_threshold); >>>> +                         ctx->check_pages_threshold, >>>> +                         ctx->device_private_page_owner); >>>>       if (chunk_size == LONG_MAX) { >>>>           err = -EINVAL; >>>>           goto err_notifier_remove; >>>> @@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm >>>> *gpusvm, >>>>           .notifier = notifier, >>>>           .start = pages_start, >>>>           .end = pages_end, >>>> -        .dev_private_owner = gpusvm->device_private_page_owner, >>>> +        .dev_private_owner = ctx->device_private_page_owner, >>>>       }; >>>>       void *zdd; >>>>       unsigned long timeout = >>>> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c >>>> index 7f2f1f041f1d..7e2db71ff34e 100644 >>>> --- a/drivers/gpu/drm/xe/xe_svm.c >>>> +++ b/drivers/gpu/drm/xe/xe_svm.c >>>> @@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range >>>> *range, const char *operation) >>>>       range_debug(range, operation); >>>>   } >>>>   -static void *xe_svm_devm_owner(struct xe_device *xe) >>>> -{ >>>> -    return xe; >>>> -} >>>> - >>>>   static struct drm_gpusvm_range * >>>>   xe_svm_range_alloc(struct drm_gpusvm *gpusvm) >>>>   { >>>> @@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm) >>>>                 xe_svm_garbage_collector_work_func); >>>>             err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM", >>>> &vm->xe->drm, >>>> -                      current->mm, xe_svm_devm_owner(vm->xe), 0, >>>> -                      vm->size, >>>> +                      current->mm, 0, vm->size, >>>>                         xe_modparam.svm_notifier_size * SZ_1M, >>>>                         &gpusvm_ops, fault_chunk_sizes, >>>>                         ARRAY_SIZE(fault_chunk_sizes)); >>>>           drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock); >>>>       } else { >>>>           err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)", >>>> -                      &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL, >>>> +                      &vm->xe->drm, NULL, 0, 0, 0, NULL, >>>>                         NULL, 0); >>>>       } >>>>   @@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct >>>> xe_vm *vm, struct xe_vma *vma, >>>>           .devmem_only = need_vram && devmem_possible, >>>>           .timeslice_ms = need_vram && devmem_possible ? >>>>               vm->xe->atomic_svm_timeslice_ms : 0, >>>> +        .device_private_page_owner = xe_svm_devm_owner(vm->xe), >>>>       }; >>>>       struct xe_validation_ctx vctx; >>>>       struct drm_exec exec; >>>> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h >>>> index cef6ee7d6fe3..0955d2ac8d74 100644 >>>> --- a/drivers/gpu/drm/xe/xe_svm.h >>>> +++ b/drivers/gpu/drm/xe/xe_svm.h >>>> @@ -6,6 +6,20 @@ >>>>   #ifndef _XE_SVM_H_ >>>>   #define _XE_SVM_H_ >>>>   +struct xe_device; >>>> + >>>> +/** >>>> + * xe_svm_devm_owner() - Return the owner of device private memory >>>> + * @xe: The xe device. >>>> + * >>>> + * Return: The owner of this device's device private memory to use in >>>> + * hmm_range_fault()- >>>> + */ >>>> +static inline void *xe_svm_devm_owner(struct xe_device *xe) >>>> +{ >>>> +    return xe; >>>> +} >>>> + >>>>   #if IS_ENABLED(CONFIG_DRM_XE_GPUSVM) >>>>     #include >>>> diff --git a/drivers/gpu/drm/xe/xe_userptr.c >>>> b/drivers/gpu/drm/xe/xe_userptr.c >>>> index 91d09af71ced..f16e92cd8090 100644 >>>> --- a/drivers/gpu/drm/xe/xe_userptr.c >>>> +++ b/drivers/gpu/drm/xe/xe_userptr.c >>>> @@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma >>>> *uvma) >>>>       struct xe_device *xe = vm->xe; >>>>       struct drm_gpusvm_ctx ctx = { >>>>           .read_only = xe_vma_read_only(vma), >>>> +        .device_private_page_owner = NULL, >>>>       }; >>>>         lockdep_assert_held(&vm->lock); >>>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c >>>> index 80b7f13ecd80..4e914928e0a9 100644 >>>> --- a/drivers/gpu/drm/xe/xe_vm.c >>>> +++ b/drivers/gpu/drm/xe/xe_vm.c >>>> @@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm, >>>> struct xe_vma_op *op) >>>>       ctx.read_only = xe_vma_read_only(vma); >>>>       ctx.devmem_possible = devmem_possible; >>>>       ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0; >>>> +    ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe); >>>>         /* TODO: Threading the migration */ >>>>       xa_for_each(&op->prefetch_range.range, i, svm_range) { >>>> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h >>>> index 5434048a2ca4..b92faa9a26b2 100644 >>>> --- a/include/drm/drm_gpusvm.h >>>> +++ b/include/drm/drm_gpusvm.h >>>> @@ -179,7 +179,6 @@ struct drm_gpusvm_range { >>>>    * @name: Name of the GPU SVM >>>>    * @drm: Pointer to the DRM device structure >>>>    * @mm: Pointer to the mm_struct for the address space >>>> - * @device_private_page_owner: Device private pages owner >>>>    * @mm_start: Start address of GPU SVM >>>>    * @mm_range: Range of the GPU SVM >>>>    * @notifier_size: Size of individual notifiers >>>> @@ -204,7 +203,6 @@ struct drm_gpusvm { >>>>       const char *name; >>>>       struct drm_device *drm; >>>>       struct mm_struct *mm; >>>> -    void *device_private_page_owner; >>>>       unsigned long mm_start; >>>>       unsigned long mm_range; >>>>       unsigned long notifier_size; >>>> @@ -226,6 +224,8 @@ struct drm_gpusvm { >>>>   /** >>>>    * struct drm_gpusvm_ctx - DRM GPU SVM context >>>>    * >>>> + * @device_private_page_owner: The device-private page owner to use for >>>> + * this operation >>>>    * @check_pages_threshold: Check CPU pages for present if chunk is >>>> less than or >>>>    *                         equal to threshold. If not present, >>>> reduce chunk >>>>    *                         size. >>>> @@ -239,6 +239,7 @@ struct drm_gpusvm { >>>>    * Context that is DRM GPUSVM is operating in (i.e. user arguments). >>>>    */ >>>>   struct drm_gpusvm_ctx { >>>> +    void *device_private_page_owner; >>>>       unsigned long check_pages_threshold; >>>>       unsigned long timeslice_ms; >>>>       unsigned int in_notifier :1; >>>> @@ -249,7 +250,7 @@ struct drm_gpusvm_ctx { >>>>     int drm_gpusvm_init(struct drm_gpusvm *gpusvm, >>>>               const char *name, struct drm_device *drm, >>>> -            struct mm_struct *mm, void *device_private_page_owner, >>>> +            struct mm_struct *mm, >>>>               unsigned long mm_start, unsigned long mm_range, >>>>               unsigned long notifier_size, >>>>               const struct drm_gpusvm_ops *ops, >>>