Re: [RFC PATCH 3/3] drm/virtio: implement blob userptr resource object

[Simona Vetter <simona.vetter@ffwll.ch>]

On Fri, Dec 27, 2024 at 10:24:29AM +0800, Huang, Honglei1 wrote:
> 
> On 2024/12/22 9:59, Demi Marie Obenour wrote:
> > On 12/20/24 10:35 AM, Simona Vetter wrote:
> > > On Fri, Dec 20, 2024 at 06:04:09PM +0800, Honglei Huang wrote:
> > > > From: Honglei Huang <Honglei1.Huang@amd.com>
> > > > 
> > > > A virtio-gpu userptr is based on HMM notifier.
> > > > Used for let host access guest userspace memory and
> > > > notice the change of userspace memory.
> > > > This series patches are in very beginning state,
> > > > User space are pinned currently to ensure the host
> > > > device memory operations are correct.
> > > > The free and unmap operations for userspace can be
> > > > handled by MMU notifier this is a simple and basice
> > > > SVM feature for this series patches.
> > > > The physical PFNS update operations is splited into
> > > > two OPs in here. The evicted memories won't be used
> > > > anymore but remap into host again to achieve same
> > > > effect with hmm_rang_fault.
> > > 
> > > So in my opinion there are two ways to implement userptr that make sense:
> > > 
> > > - pinned userptr with pin_user_pages(FOLL_LONGTERM). there is not mmu
> > >    notifier
> > > 
> > > - unpinnned userptr where you entirely rely on userptr and do not hold any
> > >    page references or page pins at all, for full SVM integration. This
> > >    should use hmm_range_fault ideally, since that's the version that
> > >    doesn't ever grab any page reference pins.
> > > 
> > > All the in-between variants are imo really bad hacks, whether they hold a
> > > page reference or a temporary page pin (which seems to be what you're
> > > doing here). In much older kernels there was some justification for them,
> > > because strange stuff happened over fork(), but with FOLL_LONGTERM this is
> > > now all sorted out. So there's really only fully pinned, or true svm left
> > > as clean design choices imo.
> > > 
> > > With that background, why does pin_user_pages(FOLL_LONGTERM) not work for
> > > you?
> > 
> > +1 on using FOLL_LONGTERM.  Fully dynamic memory management has a huge cost
> > in complexity that pinning everything avoids.  Furthermore, this avoids the
> > host having to take action in response to guest memory reclaim requests.
> > This avoids additional complexity (and thus attack surface) on the host side.
> > Furthermore, since this is for ROCm and not for graphics, I am less concerned
> > about supporting systems that require swappable GPU VRAM.
> 
> Hi Sima and Demi,
> 
> I totally agree the flag FOLL_LONGTERM is needed, I will add it in next
> version.
> 
> And for the first pin variants implementation, the MMU notifier is also
> needed I think.Cause the userptr feature in UMD generally used like this:
> the registering of userptr always is explicitly invoked by user code like
> "registerMemoryToGPU(userptrAddr, ...)", but for the userptr release/free,
> there is no explicit API for it, at least in hsakmt/KFD stack. User just
> need call system call "free(userptrAddr)", then kernel driver will release
> the userptr by MMU notifier callback.Virtio-GPU has no other way to know if
> user has been free the userptr except for MMU notifior.And in UMD theres is
> no way to get the free() operation is invoked by user.The only way is use
> MMU notifier in virtio-GPU driver and free the corresponding data in host by
> some virtio CMDs as far as I can see.
> 
> And for the second way that is use hmm_range_fault, there is a predictable
> issues as far as I can see, at least in hsakmt/KFD stack. That is the memory
> may migrate when GPU/device is working. In bare metal, when memory is
> migrating KFD driver will pause the compute work of the device in
> mmap_wirte_lock then use hmm_range_fault to remap the migrated/evicted
> memories to GPU then restore the compute work of device to ensure the
> correction of the data. But in virtio-GPU driver the migration happen in
> guest kernel, the evict mmu notifier callback happens in guest, a virtio CMD
> can be used for notify host but as lack of mmap_write_lock protection in
> host kernel, host will hold invalid data for a short period of time, this
> may lead to some issues. And it is hard to fix as far as I can see.
> 
> I will extract some APIs into helper according to your request, and I will
> refactor the whole userptr implementation, use some callbacks in page
> getting path, let the pin method and hmm_range_fault can be choiced
> in this series patches.

Ok, so if this is for svm, then you need full blast hmm, or the semantics
are buggy. You cannot fake svm with pin(FOLL_LONGTERM) userptr, this does
not work.

The other option is that hsakmt/kfd api is completely busted, and that's
kinda not a kernel problem.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch