From patchwork Thu Jan 25 14:06:48 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thierry Reding X-Patchwork-Id: 10184101 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B63DE601D5 for ; Thu, 25 Jan 2018 14:06:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A23E228763 for ; Thu, 25 Jan 2018 14:06:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 95AEE28A87; Thu, 25 Jan 2018 14:06:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A90E328763 for ; Thu, 25 Jan 2018 14:06:56 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 564686E075; Thu, 25 Jan 2018 14:06:54 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-qt0-x243.google.com (mail-qt0-x243.google.com [IPv6:2607:f8b0:400d:c0d::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id 154476E075 for ; Thu, 25 Jan 2018 14:06:52 +0000 (UTC) Received: by mail-qt0-x243.google.com with SMTP id s3so19387872qtb.10 for ; Thu, 25 Jan 2018 06:06:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=aPjxB7gcnBpuNEbwd3/DCFAfdWlKjbFDOWyp75fBg1I=; b=TzZy4NE7M2g33iNAm8CJi6Lzu1GdaSxWKWTC0rSXAxXhXqU7+QXw2D2s/OWU4Lm68o dL/1R13+qjJdwoJXhOvfg7vxHHLr8TphAhaoPtYIdZr8FuBslUfLNhBcsy6DvuREO14I FBKjADPJK7dKCLv+j32vXgfGczaqlkOq46NyQmNVR4J8/kE8hF1ULAZoVgZzE2oPpzrT 0sBHJMHbsPS2y2z2gWhl2XdT0NDkUv4Bx0NRZunLnDPr2JxYCqyY79E3/jHs4tkrhZMQ LDRIUAlGGrfkIVFmBez7iAnDKIMI9KS6V2JbZ8fdDOgdDK7qpHVgYx8jphjB2W+I7I+p rB2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=aPjxB7gcnBpuNEbwd3/DCFAfdWlKjbFDOWyp75fBg1I=; b=qQI7JF6E3NPo9kqEUhymRKJxB3+EF9q54/6GckEBbvz2UsLatTVpLUfl2dggIrX6Fz /V1ubdrY5vPF8lCxQa15oa0dVoLaqTfMvssM7Q2IWHHoZkki97XN0vYvOPapq5A+aDD5 /L8GNlvz7DW/c1Qh7Fa6p14Yhv/LyhQKy5EXWyT3icWkz6MK1GJNJmGtqczTS+ksO3NS qy0xha4EHPqcnLIoAih4TnOAWi9cIWT24PgXuULpOAZ9fsi0L+scpSEPzqNzHV6Zd63m 7LCC1PW7mOUZChzC9zPW7GxM/j9+5K0gznXb/OgYLUvwm3ZzR6zJJqaZvMm53leAgwA1 UPFw== X-Gm-Message-State: AKwxyteRN+i8cX44EtGHRKR6ppLQO+G2mFF+Tl1HL2OsztXuxk6T2zhK jnIfxmACWIErdEFHolm0t+k= X-Google-Smtp-Source: AH8x227DfU5uqha2h+gbRGxnXEXh4VlLVWEQ4W6pKWKFhHDYA8mFVEV8Tse8ubCHf9zKsYdRhpi5AA== X-Received: by 10.237.53.144 with SMTP id c16mr1223711qte.215.1516889211836; Thu, 25 Jan 2018 06:06:51 -0800 (PST) Received: from localhost (p200300E41F05C100DA2191AAB13561AC.dip0.t-ipconnect.de. [2003:e4:1f05:c100:da21:91aa:b135:61ac]) by smtp.gmail.com with ESMTPSA id e5sm1866745qkj.87.2018.01.25.06.06.50 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 25 Jan 2018 06:06:51 -0800 (PST) Date: Thu, 25 Jan 2018 15:06:48 +0100 From: Thierry Reding To: Russell King - ARM Linux Subject: Re: [PATCH 2/5] drm: add ARM flush implementation Message-ID: <20180125140648.GA27888@ulmo> References: <20180124025606.3020-2-gurchetansingh@chromium.org> <20180124124542.GV17719@n2100.armlinux.org.uk> <20180124192610.GA30716@n2100.armlinux.org.uk> MIME-Version: 1.0 In-Reply-To: <20180124192610.GA30716@n2100.armlinux.org.uk> User-Agent: Mutt/1.9.1 (2017-09-22) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: jeffy.chen@rock-chips.com, ML dri-devel , Gurchetan Singh , Laurent Pinchart , Daniel Vetter , linux-arm-kernel@lists.infradead.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Jan 24, 2018 at 07:26:11PM +0000, Russell King - ARM Linux wrote: > On Wed, Jan 24, 2018 at 10:45:28AM -0800, Gurchetan Singh wrote: > > On Wed, Jan 24, 2018 at 4:45 AM, Russell King - ARM Linux < > > linux@armlinux.org.uk> wrote: > > > So no, this is not an acceptable approach. > > > > > > Secondly, in light of spectre and meltdown, do we _really_ want to > > > export cache flushing to userspace in any case - these attacks rely > > > on being able to flush specific cache lines from the caches in order > > > to do the timing attacks (while leaving others in place.) > > > > > Currently, 32-bit ARM does not export such flushing capabilities to > > > userspace, which makes it very difficult (I'm not going to say > > > impossible) to get any working proof-of-code program that even > > > illustrates the timing attack. Exposing this functionality changes > > > that game, and means that we're much more open to these exploits. > > > (Some may say that you can flush cache lines by reading a large > > > enough buffer - I'm aware, I've tried that, the results are too > > > unreliable even for a simple attempt which doesn't involve crossing > > > privilege boundaries.) > > > > > > > Will using the DMA API (dma_sync_single_for_device / > > dma_sync_sg_for_device) mitigate your Meltdown / Spectre concerns in any > > way? > > I see no point in answering that question based on what you've written > below (see below for why). > > > > Do you really need cacheable GPU buffers, or will write combining > > > buffers (as we use elsewhere such as etnaviv) suffice? Please provide > > > some _real_ _world_ performance measurements that demonstrate that > > > there is a real need for this functionality. > > > > > > My desire is for the vgem driver to work correctly on ARM, which requires > > cache flushing. The mappings vgem itself creates are write combine. > > If the pages are mapped write-combine, they are by definition *not* > cacheable, so there should be no cache flushing required. > > > The > > issue is the pages retrieved on ARM architecture usually have to be flushed > > before they can be used (see rockchip_gem_get_pages / tegra_bo_get_pages). > > This patch set attempts to do the flushing in an architecture independent > > manner (since vgem is intended to work on ARM / x86). > > I see rockchip_gem_get_pages() using shmem_read_mapping_page() to get > the pages. That's more or less fine, we do that on Etnaviv too. > > (Side note: provided the pages are not coming from lowmem, as mapping > lowmem pages are mapped cacheable, and if you also map them elsewhere > as write-combine, you're stepping into some potential cache attribute > issues.) > > How we deal with this in Etnaviv is to use dma_map_sg() after we get > the pages - see > > etnaviv_gem_get_pages(), which calls the memory specific .get_pages > method, and goes on to call etnaviv_gem_scatter_map(). I think I'm to blame for this. Back at the time the patch was based on my incomplete understanding of the DMA API. It's also possible that it wasn't working at the time because the DMA/IOMMU glue wasn't quite the same as it is today. I have a vague recollection that I tried using dma_map_sg() and it was creating a second IOVA mapping (in addition to the one we explicitly create in the driver with the IOMMU API). However, that's no longer happening today, so I ended up doing something very similar to etnaviv. I've got the below patch queued for v4.17 and I think the same should work for both Rockchip and VGEM. Thierry --- >8 --- From 0f83d1aefcd0ca49c88d483a4161e3a02b5d1f32 Mon Sep 17 00:00:00 2001 From: Thierry Reding Date: Wed, 13 Dec 2017 12:22:48 +0100 Subject: [PATCH] drm/tegra: gem: Map pages via the DMA API When allocating pages, map them with the DMA API in order to invalidate caches. This is the correct usage of the API and works just as well as faking up the SG table and using the dma_sync_sg_for_device() function. Signed-off-by: Thierry Reding --- drivers/gpu/drm/tegra/gem.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 1bc5c6d1e5b5..8ab6057808e6 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -203,6 +203,8 @@ static struct tegra_bo *tegra_bo_alloc_object(struct drm_device *drm, static void tegra_bo_free(struct drm_device *drm, struct tegra_bo *bo) { if (bo->pages) { + dma_unmap_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents, + DMA_BIDIRECTIONAL); drm_gem_put_pages(&bo->gem, bo->pages, true, true); sg_free_table(bo->sgt); kfree(bo->sgt); @@ -213,8 +215,7 @@ static void tegra_bo_free(struct drm_device *drm, struct tegra_bo *bo) static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo) { - struct scatterlist *s; - unsigned int i; + int err; bo->pages = drm_gem_get_pages(&bo->gem); if (IS_ERR(bo->pages)) @@ -223,27 +224,26 @@ static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo) bo->num_pages = bo->gem.size >> PAGE_SHIFT; bo->sgt = drm_prime_pages_to_sg(bo->pages, bo->num_pages); - if (IS_ERR(bo->sgt)) + if (IS_ERR(bo->sgt)) { + err = PTR_ERR(bo->sgt); goto put_pages; + } - /* - * Fake up the SG table so that dma_sync_sg_for_device() can be used - * to flush the pages associated with it. - * - * TODO: Replace this by drm_clflash_sg() once it can be implemented - * without relying on symbols that are not exported. - */ - for_each_sg(bo->sgt->sgl, s, bo->sgt->nents, i) - sg_dma_address(s) = sg_phys(s); - - dma_sync_sg_for_device(drm->dev, bo->sgt->sgl, bo->sgt->nents, - DMA_TO_DEVICE); + err = dma_map_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents, + DMA_BIDIRECTIONAL); + if (err == 0) { + err = -EFAULT; + goto free_sgt; + } return 0; +free_sgt: + sg_free_table(bo->sgt); + kfree(bo->sgt); put_pages: drm_gem_put_pages(&bo->gem, bo->pages, false, false); - return PTR_ERR(bo->sgt); + return err; } static int tegra_bo_alloc(struct drm_device *drm, struct tegra_bo *bo)