lkml.org 
[lkml]   [2022]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@vger.kernel.org
    > > Cc: Trigger Huang <Trigger.Huang@gmail.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@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
    http://blog.ffwll.ch

    \
     
     \ /
      Last update: 2022-09-06 22:08    [W:2.980 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site