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@xxxxxxxxxxxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
