[PATCH] drm/i915/guc: Document that the ads blob entries only lie within the first page

[PATCH] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Chris Wilson]

guc_addon_create() makes the assumption that it need only kmap the
initial page in order to write all of the configuration data used by the
guc. Confusingly it also allocates many scratch pages in the same vma
and passes that to the guc. Reassure the reader that all is well with a
BUILD_BUG_ON() that we do not access outside of the kmapped page.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 5 +++++
 drivers/gpu/drm/i915/i915_utils.h          | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 97726fcb1230..91d7ab0df0cd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -888,13 +888,16 @@ static void guc_addon_create(struct intel_guc *guc)
 		guc->ads_vma = vma;
 	}
 
+	/* First members are assumed to be in a single page */
 	page = i915_vma_first_page(vma);
 	blob = kmap(page);
 
 	/* GuC scheduling policies */
+	BUILD_BUG_ON(ptr_offset_end(blob, policies) > PAGE_SIZE);
 	guc_policies_init(&blob->policies);
 
 	/* MMIO reg state */
+	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);
 	for_each_engine(engine, dev_priv, id) {
 		blob->reg_state.mmio_white_list[engine->guc_id].mmio_start =
 			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
@@ -903,6 +906,8 @@ static void guc_addon_create(struct intel_guc *guc)
 		blob->reg_state.mmio_white_list[engine->guc_id].count = 0;
 	}
 
+	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);
+
 	/*
 	 * The GuC requires a "Golden Context" when it reinitialises
 	 * engines after a reset. Here we use the Render ring default
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 94a3a3299910..2976bf9d94b4 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -67,6 +67,7 @@
 	((typeof(ptr))((unsigned long)(ptr) | (bits)))
 
 #define ptr_offset(ptr, member) offsetof(typeof(*(ptr)), member)
+#define ptr_offset_end(ptr, member) offsetofend(typeof(*(ptr)), member)
 
 #define fetch_and_zero(ptr) ({						\
 	typeof(*ptr) __T = *(ptr);					\
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Chris Wilson]

On Thu, Mar 16, 2017 at 10:55:06AM +0000, Chris Wilson wrote:
> guc_addon_create() makes the assumption that it need only kmap the
> initial page in order to write all of the configuration data used by the
> guc. Confusingly it also allocates many scratch pages in the same vma
> and passes that to the guc. Reassure the reader that all is well with a
> BUILD_BUG_ON() that we do not access outside of the kmapped page.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 5 +++++
>  drivers/gpu/drm/i915/i915_utils.h          | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 97726fcb1230..91d7ab0df0cd 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -888,13 +888,16 @@ static void guc_addon_create(struct intel_guc *guc)
>  		guc->ads_vma = vma;
>  	}
>  
> +	/* First members are assumed to be in a single page */

s/First/Written/

>  	page = i915_vma_first_page(vma);
>  	blob = kmap(page);
>  
>  	/* GuC scheduling policies */
> +	BUILD_BUG_ON(ptr_offset_end(blob, policies) > PAGE_SIZE);
>  	guc_policies_init(&blob->policies);
>  
>  	/* MMIO reg state */
> +	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);
>  	for_each_engine(engine, dev_priv, id) {
>  		blob->reg_state.mmio_white_list[engine->guc_id].mmio_start =
>  			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> @@ -903,6 +906,8 @@ static void guc_addon_create(struct intel_guc *guc)
>  		blob->reg_state.mmio_white_list[engine->guc_id].count = 0;
>  	}
>  
> +	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);
> +
>  	/*
>  	 * The GuC requires a "Golden Context" when it reinitialises
>  	 * engines after a reset. Here we use the Render ring default

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Michal Wajdeczko]

On Thu, Mar 16, 2017 at 10:58:20AM +0000, Chris Wilson wrote:
> On Thu, Mar 16, 2017 at 10:55:06AM +0000, Chris Wilson wrote:
> > guc_addon_create() makes the assumption that it need only kmap the
> > initial page in order to write all of the configuration data used by the
> > guc. Confusingly it also allocates many scratch pages in the same vma
> > and passes that to the guc. Reassure the reader that all is well with a
> > BUILD_BUG_ON() that we do not access outside of the kmapped page.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 5 +++++
> >  drivers/gpu/drm/i915/i915_utils.h          | 1 +
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 97726fcb1230..91d7ab0df0cd 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -888,13 +888,16 @@ static void guc_addon_create(struct intel_guc *guc)
> >  		guc->ads_vma = vma;
> >  	}
> >  
> > +	/* First members are assumed to be in a single page */
> 
> s/First/Written/

Btw, maybe it would be better to move all these BUILD_BUGs here?

> 
> >  	page = i915_vma_first_page(vma);
> >  	blob = kmap(page);
> >  
> >  	/* GuC scheduling policies */
> > +	BUILD_BUG_ON(ptr_offset_end(blob, policies) > PAGE_SIZE);
> >  	guc_policies_init(&blob->policies);
> >  
> >  	/* MMIO reg state */
> > +	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);
> >  	for_each_engine(engine, dev_priv, id) {
> >  		blob->reg_state.mmio_white_list[engine->guc_id].mmio_start =
> >  			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> > @@ -903,6 +906,8 @@ static void guc_addon_create(struct intel_guc *guc)
> >  		blob->reg_state.mmio_white_list[engine->guc_id].count = 0;
> >  	}
> >  
> > +	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);

s/reg_state/ads


-Michal

> > +
> >  	/*
> >  	 * The GuC requires a "Golden Context" when it reinitialises
> >  	 * engines after a reset. Here we use the Render ring default
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Chris Wilson]

On Thu, Mar 16, 2017 at 12:06:20PM +0100, Michal Wajdeczko wrote:
> On Thu, Mar 16, 2017 at 10:58:20AM +0000, Chris Wilson wrote:
> > On Thu, Mar 16, 2017 at 10:55:06AM +0000, Chris Wilson wrote:
> > > guc_addon_create() makes the assumption that it need only kmap the
> > > initial page in order to write all of the configuration data used by the
> > > guc. Confusingly it also allocates many scratch pages in the same vma
> > > and passes that to the guc. Reassure the reader that all is well with a
> > > BUILD_BUG_ON() that we do not access outside of the kmapped page.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_guc_submission.c | 5 +++++
> > >  drivers/gpu/drm/i915/i915_utils.h          | 1 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > index 97726fcb1230..91d7ab0df0cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > @@ -888,13 +888,16 @@ static void guc_addon_create(struct intel_guc *guc)
> > >  		guc->ads_vma = vma;
> > >  	}
> > >  
> > > +	/* First members are assumed to be in a single page */
> > 
> > s/First/Written/
> 
> Btw, maybe it would be better to move all these BUILD_BUGs here?

I was thinking next to the write so that if they were changed, copied,
it would be more obvious to update; as well as the check being clearly
associated with the write.
-Chris


-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Michal Wajdeczko]

On Thu, Mar 16, 2017 at 11:18:12AM +0000, Chris Wilson wrote:
> On Thu, Mar 16, 2017 at 12:06:20PM +0100, Michal Wajdeczko wrote:
> > On Thu, Mar 16, 2017 at 10:58:20AM +0000, Chris Wilson wrote:
> > > On Thu, Mar 16, 2017 at 10:55:06AM +0000, Chris Wilson wrote:
> > > > guc_addon_create() makes the assumption that it need only kmap the
> > > > initial page in order to write all of the configuration data used by the
> > > > guc. Confusingly it also allocates many scratch pages in the same vma
> > > > and passes that to the guc. Reassure the reader that all is well with a
> > > > BUILD_BUG_ON() that we do not access outside of the kmapped page.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_guc_submission.c | 5 +++++
> > > >  drivers/gpu/drm/i915/i915_utils.h          | 1 +
> > > >  2 files changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > index 97726fcb1230..91d7ab0df0cd 100644
> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > @@ -888,13 +888,16 @@ static void guc_addon_create(struct intel_guc *guc)
> > > >  		guc->ads_vma = vma;
> > > >  	}
> > > >  
> > > > +	/* First members are assumed to be in a single page */
> > > 
> > > s/First/Written/
> > 
> > Btw, maybe it would be better to move all these BUILD_BUGs here?
> 
> I was thinking next to the write so that if they were changed, copied,
> it would be more obvious to update; as well as the check being clearly
> associated with the write.

Your intention was clear ;)
But, since they would be spread across over the function, it would be
easy to forget about the origin of these BUILD_BUGs as they will be far
from the introduction comment ... One can easily forget to add them for
new fields, or even drop on rename or something.
Note that by moving them here, we also make this comment more visible
and associated with strong enforcement (at least to the level that we
can handle today)

-Michal


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Chris Wilson]

guc_addon_create() makes the assumption that it need only kmap the
initial page in order to write all of the configuration data used by the
guc. Confusingly it also allocates many scratch pages in the same vma
and passes that to the guc. Reassure the reader that all is well with a
BUILD_BUG_ON() that we do not access outside of the kmapped page.

v2: Fix check against ads entry

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
 drivers/gpu/drm/i915/i915_utils.h          | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 97726fcb1230..97ac04a823aa 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -888,13 +888,17 @@ static void guc_addon_create(struct intel_guc *guc)
 		guc->ads_vma = vma;
 	}
 
+	/* Written members are assumed to be in a single page */
+	BUILD_BUG_ON(ptr_offset(blob, reg_state_buffer) > PAGE_SIZE);
 	page = i915_vma_first_page(vma);
 	blob = kmap(page);
 
 	/* GuC scheduling policies */
+	BUILD_BUG_ON(ptr_offset_end(blob, policies) > PAGE_SIZE);
 	guc_policies_init(&blob->policies);
 
 	/* MMIO reg state */
+	BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);
 	for_each_engine(engine, dev_priv, id) {
 		blob->reg_state.mmio_white_list[engine->guc_id].mmio_start =
 			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
@@ -903,6 +907,8 @@ static void guc_addon_create(struct intel_guc *guc)
 		blob->reg_state.mmio_white_list[engine->guc_id].count = 0;
 	}
 
+	BUILD_BUG_ON(ptr_offset_end(blob, ads) > PAGE_SIZE);
+
 	/*
 	 * The GuC requires a "Golden Context" when it reinitialises
 	 * engines after a reset. Here we use the Render ring default
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 94a3a3299910..2976bf9d94b4 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -67,6 +67,7 @@
 	((typeof(ptr))((unsigned long)(ptr) | (bits)))
 
 #define ptr_offset(ptr, member) offsetof(typeof(*(ptr)), member)
+#define ptr_offset_end(ptr, member) offsetofend(typeof(*(ptr)), member)
 
 #define fetch_and_zero(ptr) ({						\
 	typeof(*ptr) __T = *(ptr);					\
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/guc: Document that the ads blob entries only lie within the first page

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Document that the ads blob entries only lie within the first page
URL   : https://patchwork.freedesktop.org/series/21372/
State : success

== Summary ==

Series 21372v1 drm/i915/guc: Document that the ads blob entries only lie within the first page
https://patchwork.freedesktop.org/api/1.0/series/21372/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 467s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 583s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 533s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 566s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 500s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 500s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 439s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 431s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 435s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 520s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 499s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 475s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 595s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 482s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 516s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 547s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 423s
fi-kbl-7500u failed to connect after reboot

10fb1e7b1a08adce1b8db7186c5eb28dea8a2b97 drm-tip: 2017y-03m-16d-10h-44m-23s UTC integration manifest
9206caf drm/i915/guc: Document that the ads blob entries only lie within the first page

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4197/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915/guc: Document that the ads blob entries only lie within the first page (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Document that the ads blob entries only lie within the first page (rev2)
URL   : https://patchwork.freedesktop.org/series/21372/
State : failure

== Summary ==

Series 21372v2 drm/i915/guc: Document that the ads blob entries only lie within the first page
https://patchwork.freedesktop.org/api/1.0/series/21372/revisions/2/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-snb-2600)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 464s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 583s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 530s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 575s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 499s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 498s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 438s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 429s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 441s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 518s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 506s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 483s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 601s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 492s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 529s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 549s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 429s
fi-kbl-7500u failed to connect after reboot

10fb1e7b1a08adce1b8db7186c5eb28dea8a2b97 drm-tip: 2017y-03m-16d-10h-44m-23s UTC integration manifest
d384dae drm/i915/guc: Document that the ads blob entries only lie within the first page

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4198/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Michal Wajdeczko]

On Thu, Mar 16, 2017 at 11:41:51AM +0000, Chris Wilson wrote:
> guc_addon_create() makes the assumption that it need only kmap the
> initial page in order to write all of the configuration data used by the
> guc. Confusingly it also allocates many scratch pages in the same vma
> and passes that to the guc. Reassure the reader that all is well with a
> BUILD_BUG_ON() that we do not access outside of the kmapped page.
> 
> v2: Fix check against ads entry
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
>  drivers/gpu/drm/i915/i915_utils.h          | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 97726fcb1230..97ac04a823aa 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -888,13 +888,17 @@ static void guc_addon_create(struct intel_guc *guc)
>  		guc->ads_vma = vma;
>  	}
>  
> +	/* Written members are assumed to be in a single page */
> +	BUILD_BUG_ON(ptr_offset(blob, reg_state_buffer) > PAGE_SIZE);

Hmm, this is not very intuitive, and may still provide false results in the future.

What about introducing fake field that will mark the border between what's we can write what not

	struct {
		struct guc_ads ads;
		struct guc_policies policies;
		struct guc_mmio_reg_state reg_state;
	+	struct { } end_touchable;
		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
	} __packed *blob;

	+ BUILD_BUG_ON(ptr_offset(blob, end_touchable) > PAGE_SIZE);

Alternatively, wrap writtable members in substruct

	struct {
	+	struct {
			struct guc_ads ads;
			struct guc_policies policies;
			struct guc_mmio_reg_state reg_state;
	+	} touchable;
		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
	} __packed *blob;

	+ BUILD_BUG_ON(ptr_offset_end(blob, touchable) > PAGE_SIZE);

But I still believe option to put all BUILD_BUGs together is simplest, easy and clear:

	+ /* Written members are assumed to be in a single page */
	+ BUILD_BUG_ON(ptr_offset_end(blob, ads) > PAGE_SIZE);
	+ BUILD_BUG_ON(ptr_offset_end(blob, policies) > PAGE_SIZE);
	+ BUILD_BUG_ON(ptr_offset_end(blob, reg_state) > PAGE_SIZE);

and to catch any missing future field we can add

	+ BUILD_BUG_ON(ptr_offset_end(blob, reg_state) != ptr_offset(blob, reg_state_buffer));


-Michal

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/guc: Document that the ads blob entries only lie within the first page

[Joonas Lahtinen]

On to, 2017-03-16 at 13:31 +0100, Michal Wajdeczko wrote:
> On Thu, Mar 16, 2017 at 11:41:51AM +0000, Chris Wilson wrote:
> > 
> > guc_addon_create() makes the assumption that it need only kmap the
> > initial page in order to write all of the configuration data used by the
> > guc. Confusingly it also allocates many scratch pages in the same vma
> > and passes that to the guc. Reassure the reader that all is well with a
> > BUILD_BUG_ON() that we do not access outside of the kmapped page.
> > 
> > v2: Fix check against ads entry
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
> >  drivers/gpu/drm/i915/i915_utils.h          | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 97726fcb1230..97ac04a823aa 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -888,13 +888,17 @@ static void guc_addon_create(struct intel_guc *guc)
> >  		guc->ads_vma = vma;
> >  	}
> >  
> > +	/* Written members are assumed to be in a single page */
> > +	BUILD_BUG_ON(ptr_offset(blob, reg_state_buffer) > PAGE_SIZE);
> 
> Hmm, this is not very intuitive, and may still provide false results in the future.
> 
> What about introducing fake field that will mark the border between what's we can write what not
> 
> 	struct {
> 		struct guc_ads ads;
> 		struct guc_policies policies;
> 		struct guc_mmio_reg_state reg_state;
> 	+	struct { } end_touchable;
> 		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> 	} __packed *blob;
> 
> 	+ BUILD_BUG_ON(ptr_offset(blob, end_touchable) > PAGE_SIZE);
> 

I'd make reg_state_buffer[0] and then BUILD_BUG_ON(sizeof(*blob) >
PAGE_SIZE); We're not going to touch reg_state_buffer contents anyway.
Also, as we're not making GuC suspend yet, I wouldn't be surprised if
reg_state_buffer was supposed to be starting at PAGE_SIZE alignment.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx