Re: [PATCH] [v2] drm/i915: Paranoia - get zeroed page table pages

[Ben Widawsky <benjamin.widawsky@intel.com>]

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 <ben@bwidawsk.net>
> > ---
> >  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