[Intel-gfx] [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 at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at 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