From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH] [v2] drm/i915: Paranoia - get zeroed page table pages Date: Wed, 5 Mar 2014 08:46:16 -0800 Message-ID: <20140305164616.GA19373@intel.com> References: <1393525802-978-1-git-send-email-benjamin.widawsky@intel.com> <1393559258-1405-1-git-send-email-benjamin.widawsky@intel.com> <1394037126.31112.30.camel@intelbox> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pd0-f181.google.com (mail-pd0-f181.google.com [209.85.192.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 4FE8BFA795 for ; Wed, 5 Mar 2014 08:46:19 -0800 (PST) Received: by mail-pd0-f181.google.com with SMTP id p10so1257999pdj.26 for ; Wed, 05 Mar 2014 08:46:19 -0800 (PST) Content-Disposition: inline In-Reply-To: <1394037126.31112.30.camel@intelbox> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Imre Deak Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Wed, Mar 05, 2014 at 06:32:06PM +0200, Imre Deak wrote: > On Thu, 2014-02-27 at 19:47 -0800, Ben Widawsky wrote: > > We normally clear the page tables as one of the first things during > > initialization. They are however wired up (and potentially valid) before > > we clear them. > > I might be missing something, but afaics the page directories/tables are > not in use until after ppgtt->enable()/mm_switch() is called on them, > which is after the clear_range() call. > > I'd understand if it's about leaving uninitialized stuff _after_ > clear_range() is called. But I think because of the 1G size alignment > for ppgtt that's not possible either. > > --Imre The only case I was able to fathom was if we accidentally connect a PDE before we populate the page table. I felt it was a rather harmless patch though. I do agree with the IRC conversation that it shouldn't happen. It was in lines with the same reason of why we never BUG_ON. > > > To prevent the GPU from doing anything we might later regret, simply get > > zeroed pages, which always mean invalid on all GENs. > > > > NOTE: that a similar paranoia could be applied to GGTT via making sure > > all entries are invalid ASAP. I think the extra work required to fix > > such a BIOS bug is unwarranted until proven necessary. > > > > v2: Remove useless GFP_ZERO in the kcallocs > > > > Signed-off-by: Ben Widawsky > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 0c27d8a..5e3957e 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -359,7 +359,7 @@ static struct page **__gen8_alloc_page_tables(void) > > return ERR_PTR(-ENOMEM); > > > > for (i = 0; i < GEN8_PDES_PER_PAGE; i++) { > > - pt_pages[i] = alloc_page(GFP_KERNEL); > > + pt_pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO); > > if (!pt_pages[i]) > > goto bail; > > } > > @@ -421,7 +421,8 @@ static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt) > > static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt, > > const int max_pdp) > > { > > - ppgtt->pd_pages = alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_SHIFT)); > > + ppgtt->pd_pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, > > + get_order(max_pdp << PAGE_SHIFT)); > > if (!ppgtt->pd_pages) > > return -ENOMEM; > > > > @@ -1021,7 +1022,7 @@ static int gen6_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt) > > return -ENOMEM; > > > > for (i = 0; i < ppgtt->num_pd_entries; i++) { > > - ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL); > > + ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO); > > if (!ppgtt->pt_pages[i]) { > > gen6_ppgtt_free(ppgtt); > > return -ENOMEM; > -- Ben Widawsky, Intel Open Source Technology Center