[Mesa-dev] [PATCH] i965: Revert absolute mode for constant buffer pointers.

[Kristian Høgsberg]

On Thu, Oct 19, 2017 at 4:18 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> The kernel doesn't initialize the value of the INSTPM or CS_DEBUG_MODE2
> registers at context initialization time.  Instead, they're inherited
> from whatever happened to be running on the GPU prior to first run of a
> new context.  So, when we started setting these, other contexts in the
> system started inheriting our values.  Since this controls whether
> 3DSTATE_CONSTANT_* takes a pointer or an offset, getting the wrong
> setting is fatal for almost any process which isn't expecting this.
>
> Unfortunately, VA-API and Beignet don't initialize this (nor does older
> Mesa), so they will die horribly if we start doing this.  UXA and SNA
> don't use any push constants, so they are unaffected.
>
> The kernel developers are apparently uninterested in making the proto-
> context initialize these registers to guarantee deterministic behavior.

Could somebody from the kernel team elaborate here? This is obviously
broken and undermines the security and containerization that hw
contexts are supposed to provide. I'm really curious what the thinking
is here...

Kristian

> Apparently, the "Default Value" of registers in the documentation is a
> total lie, and cannot be relied upon by userspace.  So, we need to find
> a new solution.  One would be to patch VA-API and Beignet to initialize
> these, then get every distributor to ship those in tandem with the newer
> Mesa version.  I'm unclear when va-intel-driver releases might happen.
> Another option would be to hack Mesa to restore the register back to the
> historical default (relative mode) at the end of each batch.  This is
> also gross, as it has non-zero cost, and is also relying on userspace to
> be "polite" to other broken userspace.  A large part of the motivation
> for contexts was to provide proper process isolation, so we should not
> have to do this.  But, we're already doing it in some cases, because
> our kernel fixes to enforce isolation were reverted.
>
> In the meantime, I guess we'll just revert this patch and abandon using
> the feature.  It will lead to fewer pushed UBOs on Broadwell+, which may
> lead to lower performance, but I don't have any data on the impact.
>
> Cc: "17.2" <mesa-stable at lists.freedesktop.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102774
> ---
>  src/mesa/drivers/dri/i965/brw_state_upload.c | 24 ------------------------
>  src/mesa/drivers/dri/i965/intel_screen.c     |  2 +-
>  2 files changed, 1 insertion(+), 25 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c b/src/mesa/drivers/dri/i965/brw_state_upload.c
> index 16f44d03bbe..23e4ebda259 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> @@ -101,30 +101,6 @@ brw_upload_initial_gpu_state(struct brw_context *brw)
>        OUT_BATCH(0);
>        ADVANCE_BATCH();
>     }
> -
> -   /* Set the "CONSTANT_BUFFER Address Offset Disable" bit, so
> -    * 3DSTATE_CONSTANT_XS buffer 0 is an absolute address.
> -    *
> -    * On Gen6-7.5, we use an execbuf parameter to do this for us.
> -    * However, the kernel ignores that when execlists are in use.
> -    * Fortunately, we can just write the registers from userspace
> -    * on Gen8+, and they're context saved/restored.
> -    */
> -   if (devinfo->gen >= 9) {
> -      BEGIN_BATCH(3);
> -      OUT_BATCH(MI_LOAD_REGISTER_IMM | (3 - 2));
> -      OUT_BATCH(CS_DEBUG_MODE2);
> -      OUT_BATCH(REG_MASK(CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE) |
> -                CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE);
> -      ADVANCE_BATCH();
> -   } else if (devinfo->gen == 8) {
> -      BEGIN_BATCH(3);
> -      OUT_BATCH(MI_LOAD_REGISTER_IMM | (3 - 2));
> -      OUT_BATCH(INSTPM);
> -      OUT_BATCH(REG_MASK(INSTPM_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE) |
> -                INSTPM_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE);
> -      ADVANCE_BATCH();
> -   }
>  }
>
>  static inline const struct brw_tracked_state *
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index ea04a72e860..776c2898d5b 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -2510,7 +2510,7 @@ __DRIconfig **intelInitScreen2(__DRIscreen *dri_screen)
>     screen->compiler = brw_compiler_create(screen, devinfo);
>     screen->compiler->shader_debug_log = shader_debug_log_mesa;
>     screen->compiler->shader_perf_log = shader_perf_log_mesa;
> -   screen->compiler->constant_buffer_0_is_relative = devinfo->gen < 8;
> +   screen->compiler->constant_buffer_0_is_relative = true;
>     screen->compiler->supports_pull_constants = true;
>
>     screen->has_exec_fence =
> --
> 2.14.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev