* [PATCH] drm/i915: Paranoia - get zeroed page table pages
@ 2014-02-27 18:30 Ben Widawsky
2014-02-28 3:47 ` [PATCH] [v2] " Ben Widawsky
0 siblings, 1 reply; 4+ messages in thread
From: Ben Widawsky @ 2014-02-27 18:30 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky, Ben Widawsky
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.
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.
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0c27d8a..f94d39d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -354,12 +354,13 @@ static struct page **__gen8_alloc_page_tables(void)
struct page **pt_pages;
int i;
- pt_pages = kcalloc(GEN8_PDES_PER_PAGE, sizeof(struct page *), GFP_KERNEL);
+ pt_pages = kcalloc(GEN8_PDES_PER_PAGE, sizeof(struct page *),
+ GFP_KERNEL | __GFP_ZERO);
if (!pt_pages)
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;
}
@@ -410,7 +411,7 @@ static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
for (i = 0; i < ppgtt->num_pd_pages; i++) {
ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
sizeof(dma_addr_t),
- GFP_KERNEL);
+ GFP_KERNEL | __GFP_ZERO);
if (!ppgtt->gen8_pt_dma_addr[i])
return -ENOMEM;
}
@@ -421,7 +422,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;
@@ -1015,13 +1017,13 @@ static int gen6_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
int i;
ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
- GFP_KERNEL);
+ GFP_KERNEL | __GFP_ZERO);
if (!ppgtt->pt_pages)
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;
@@ -1046,7 +1048,7 @@ static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
}
ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t),
- GFP_KERNEL);
+ GFP_KERNEL | __GFP_ZERO);
if (!ppgtt->pt_dma_addr) {
drm_mm_remove_node(&ppgtt->node);
gen6_ppgtt_free(ppgtt);
--
1.9.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH] [v2] drm/i915: Paranoia - get zeroed page table pages
2014-02-27 18:30 [PATCH] drm/i915: Paranoia - get zeroed page table pages Ben Widawsky
@ 2014-02-28 3:47 ` Ben Widawsky
2014-03-05 16:32 ` Imre Deak
0 siblings, 1 reply; 4+ messages in thread
From: Ben Widawsky @ 2014-02-28 3:47 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky, Ben Widawsky
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.
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;
--
1.9.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] [v2] drm/i915: Paranoia - get zeroed page table pages
2014-02-28 3:47 ` [PATCH] [v2] " Ben Widawsky
@ 2014-03-05 16:32 ` Imre Deak
2014-03-05 16:46 ` Ben Widawsky
0 siblings, 1 reply; 4+ messages in thread
From: Imre Deak @ 2014-03-05 16:32 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky
[-- Attachment #1.1: Type: text/plain, Size: 2507 bytes --]
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
> 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;
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] [v2] drm/i915: Paranoia - get zeroed page table pages
2014-03-05 16:32 ` Imre Deak
@ 2014-03-05 16:46 ` Ben Widawsky
0 siblings, 0 replies; 4+ messages in thread
From: Ben Widawsky @ 2014-03-05 16:46 UTC (permalink / raw)
To: Imre Deak; +Cc: Intel GFX, Ben Widawsky
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-03-05 16:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-27 18:30 [PATCH] drm/i915: Paranoia - get zeroed page table pages Ben Widawsky
2014-02-28 3:47 ` [PATCH] [v2] " Ben Widawsky
2014-03-05 16:32 ` Imre Deak
2014-03-05 16:46 ` Ben Widawsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.