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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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