Messages in this thread | | | Date | Wed, 7 Sep 2022 08:48:33 +0200 | Subject | Re: [PATCH v1] drm/ttm: Refcount allocated tail pages | From | Christian König <> |
| |
Am 06.09.22 um 22:05 schrieb Daniel Vetter: > On Tue, Sep 06, 2022 at 10:01:47PM +0200, Daniel Vetter wrote: >> On Mon, Aug 15, 2022 at 12:05:19PM +0200, Christian König wrote: >>> Am 15.08.22 um 11:54 schrieb Dmitry Osipenko: >>>> Higher order pages allocated using alloc_pages() aren't refcounted and they >>>> need to be refcounted, otherwise it's impossible to map them by KVM. This >>>> patch sets the refcount of the tail pages and fixes the KVM memory mapping >>>> faults. >>>> >>>> Without this change guest virgl driver can't map host buffers into guest >>>> and can't provide OpenGL 4.5 profile support to the guest. The host >>>> mappings are also needed for enabling the Venus driver using host GPU >>>> drivers that are utilizing TTM. >>>> >>>> Based on a patch proposed by Trigger Huang. >>> Well I can't count how often I have repeated this: This is an absolutely >>> clear NAK! >>> >>> TTM pages are not reference counted in the first place and because of this >>> giving them to virgl is illegal. >>> >>> Please immediately stop this completely broken approach. We have discussed >>> this multiple times now. >> Yeah we need to get this stuff closed for real by tagging them all with >> VM_IO or VM_PFNMAP asap. > For a bit more context: Anything mapping a bo should be VM_SPECIAL. And I > think we should add the checks to the gem and dma-buf mmap functions to > validate for that, and fix all the fallout. > > Otherwise this dragon keeps resurrecting ... > > VM_SPECIAL _will_ block get_user_pages, which will block everyone from > even trying to refcount this stuff. > > Minimally we need to fix this for all ttm drivers, and it sounds like > that's still not yet the case :-( Iirc last time around some funky amdkfd > userspace was the hold-up because regressions?
My recollection is that Felix and I fixed this with a KFD specific workaround. But I can double check with Felix on Monday.
Christian.
> -Daniel > >> It seems ot be a recurring amount of fun that people try to mmap dma-buf >> and then call get_user_pages on them. >> >> Which just doesn't work. I guess this is also why Rob Clark send out that >> dma-buf patch to expos mapping information (i.e. wc vs wb vs uncached). >> >> There seems to be some serious bonghits going on :-/ >> -Daniel >> >>> Regards, >>> Christian. >>> >>>> Cc: stable@vger.kernel.org >>>> Cc: Trigger Huang <Trigger.Huang@gmail.com> >>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.collabora.com%2Fnews-and-blog%2Fblog%2F2021%2F11%2F26%2Fvenus-on-qemu-enabling-new-virtual-vulkan-driver%2F%23qcom1343&data=05%7C01%7Cchristian.koenig%40amd.com%7C37a7d9b0f91249da415b08da90432d3a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637980915471280078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XN6wFiWc6Jljekmst0aOCPSTsFLlmkUjD9F%2Fl9nluAs%3D&reserved=0 >>>> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> # AMDGPU (Qemu and crosvm) >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> drivers/gpu/drm/ttm/ttm_pool.c | 25 ++++++++++++++++++++++++- >>>> 1 file changed, 24 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c >>>> index 21b61631f73a..11e92bb149c9 100644 >>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c >>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c >>>> @@ -81,6 +81,7 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags, >>>> unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS; >>>> struct ttm_pool_dma *dma; >>>> struct page *p; >>>> + unsigned int i; >>>> void *vaddr; >>>> /* Don't set the __GFP_COMP flag for higher order allocations. >>>> @@ -93,8 +94,10 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags, >>>> if (!pool->use_dma_alloc) { >>>> p = alloc_pages(gfp_flags, order); >>>> - if (p) >>>> + if (p) { >>>> p->private = order; >>>> + goto ref_tail_pages; >>>> + } >>>> return p; >>>> } >>>> @@ -120,6 +123,23 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags, >>>> dma->vaddr = (unsigned long)vaddr | order; >>>> p->private = (unsigned long)dma; >>>> + >>>> +ref_tail_pages: >>>> + /* >>>> + * KVM requires mapped tail pages to be refcounted because put_page() >>>> + * is invoked on them in the end of the page fault handling, and thus, >>>> + * tail pages need to be protected from the premature releasing. >>>> + * In fact, KVM page fault handler refuses to map tail pages to guest >>>> + * if they aren't refcounted because hva_to_pfn_remapped() checks the >>>> + * refcount specifically for this case. >>>> + * >>>> + * In particular, unreferenced tail pages result in a KVM "Bad address" >>>> + * failure for VMMs that use VirtIO-GPU when guest's Mesa VirGL driver >>>> + * accesses mapped host TTM buffer that contains tail pages. >>>> + */ >>>> + for (i = 1; i < 1 << order; i++) >>>> + page_ref_inc(p + i); >>>> + >>>> return p; >>>> error_free: >>>> @@ -133,6 +153,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching, >>>> { >>>> unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS; >>>> struct ttm_pool_dma *dma; >>>> + unsigned int i; >>>> void *vaddr; >>>> #ifdef CONFIG_X86 >>>> @@ -142,6 +163,8 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching, >>>> if (caching != ttm_cached && !PageHighMem(p)) >>>> set_pages_wb(p, 1 << order); >>>> #endif >>>> + for (i = 1; i < 1 << order; i++) >>>> + page_ref_dec(p + i); >>>> if (!pool || !pool->use_dma_alloc) { >>>> __free_pages(p, order); >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C37a7d9b0f91249da415b08da90432d3a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637980915471280078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=bGZ1OAxL%2Fd99Nqu49soWZVqvvUKjuD6n6BKkAhMv4fs%3D&reserved=0
| |