[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YxenK8xZHC6Q4Eu4@phenom.ffwll.local>
Date: Tue, 6 Sep 2022 22:01:47 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Christian König <christian.koenig@....com>
Cc: Dmitry Osipenko <dmitry.osipenko@...labora.com>,
David Airlie <airlied@...ux.ie>, Huang Rui <ray.huang@....com>,
Daniel Vetter <daniel@...ll.ch>,
Trigger Huang <Trigger.Huang@...il.com>,
Gert Wollny <gert.wollny@...labora.com>,
Antonio Caggiano <antonio.caggiano@...labora.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Dmitry Osipenko <digetx@...il.com>, kvm@...r.kernel.org,
kernel@...labora.com, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v1] drm/ttm: Refcount allocated tail pages
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.
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@...r.kernel.org
> > Cc: Trigger Huang <Trigger.Huang@...il.com>
> > Link: https://www.collabora.com/news-and-blog/blog/2021/11/26/venus-on-qemu-enabling-new-virtual-vulkan-driver/#qcom1343
> > Tested-by: Dmitry Osipenko <dmitry.osipenko@...labora.com> # AMDGPU (Qemu and crosvm)
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.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
http://blog.ffwll.ch
Powered by blists - more mailing lists