| Message ID | 1385484699-51596-21-git-send-email-bradley.d.volkin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 26, 2013 at 08:51:37AM -0800, bradley.d.volkin@intel.com wrote: > From: Brad Volkin <bradley.d.volkin@intel.com> > > The length mask is different for each ring and the size can vary, > so we should duplicate the definition with the correct encoding > for each ring. Jumping in here since this highlights the most confusing aspect of this series - the meta patching. Please implement the command parsing infrastructure upfront and in a very small number of patches (most preferably one) that avoids having to add fixes late in the series. I think using s/S/ALLOW/ s/R/REJECT/ s/B/BLACKLIST/ s/W/WHITELIST/ makes the action much more clear, and would rather that every unsafe command starts off as REJECT. (With the whitelist/blacklisting being added as separate patches with justification as they are now.) Since we do disable the security, I would also reject all unknown/unmatched commands and make ALLOW explicit. -Chris
On Tue, Nov 26, 2013 at 10:08:48AM -0800, Chris Wilson wrote: > On Tue, Nov 26, 2013 at 08:51:37AM -0800, bradley.d.volkin@intel.com wrote: > > From: Brad Volkin <bradley.d.volkin@intel.com> > > > > The length mask is different for each ring and the size can vary, > > so we should duplicate the definition with the correct encoding > > for each ring. > > Jumping in here since this highlights the most confusing aspect of this > series - the meta patching. Please implement the command parsing > infrastructure upfront and in a very small number of patches (most > preferably one) that avoids having to add fixes late in the series. Sure. As this is my first contribution, I'm still working on how to best split up a series, so I'm happy to clean up that aspect. It sounds like the series should look more like: - All parser infrastructure and implementation (basically squash current 1-9, plus the bsearch changes, plus REJECT changes) - N patches to set commands for register whitelist and bitmask checking - Enable the parser Correct? > > I think using > s/S/ALLOW/ > s/R/REJECT/ > s/B/BLACKLIST/ > s/W/WHITELIST/ > makes the action much more clear, and would rather that every unsafe > command starts off as REJECT. (With the whitelist/blacklisting being > added as separate patches with justification as they are now.) I had split out the REJECTs to make the justification easier to find in the commit message, but I can reject them from the start too. For 'B', the term 'blacklist' to me implies a set of things that are unconditionally not allowed, whereas the 'B' commands are conditionally allowed based on the bitmask checks. Are you just asking for a readability change in expanding the action names used in the table, or are you looking for any semantic changes to what the parser checks? Or am I overthinking this comment? Brad > > Since we do disable the security, I would also reject all > unknown/unmatched commands and make ALLOW explicit. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index adc7d94..8481ef0 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -61,13 +61,6 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = { CMD( MI_REPORT_HEAD, SMI, F, 1, S ), CMD( MI_SUSPEND_FLUSH, SMI, F, 1, S ), CMD( MI_SEMAPHORE_MBOX, SMI, !F, 0xFF, R ), - CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3FF, B, - .bits = {{ - .offset = 0, - .mask = MI_GLOBAL_GTT, - .expected = 0 - }}, - .bits_count = 1 ), CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ), CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W, .reg = { .offset = 1, .mask = 0x007FFFFC } ), @@ -97,6 +90,13 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = { CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ), CMD( MI_PREDICATE, SMI, F, 1, S ), CMD( MI_TOPOLOGY_FILTER, SMI, F, 1, S ), + CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3F, B, + .bits = {{ + .offset = 0, + .mask = MI_GLOBAL_GTT, + .expected = 0 + }}, + .bits_count = 1 ), CMD( MI_CLFLUSH, SMI, !F, 0x3FF, B, .bits = {{ .offset = 0, @@ -165,6 +165,13 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = { static const struct drm_i915_cmd_descriptor video_cmds[] = { CMD( MI_ARB_ON_OFF, SMI, F, 1, R ), + CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, B, + .bits = {{ + .offset = 0, + .mask = MI_GLOBAL_GTT, + .expected = 0 + }}, + .bits_count = 1 ), CMD( MI_FLUSH_DW, SMI, !F, 0x3F, B, .bits = {{ .offset = 0, @@ -202,6 +209,13 @@ static const struct drm_i915_cmd_descriptor video_cmds[] = { static const struct drm_i915_cmd_descriptor vecs_cmds[] = { CMD( MI_ARB_ON_OFF, SMI, F, 1, R ), + CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, B, + .bits = {{ + .offset = 0, + .mask = MI_GLOBAL_GTT, + .expected = 0 + }}, + .bits_count = 1 ), CMD( MI_FLUSH_DW, SMI, !F, 0x3F, B, .bits = {{ .offset = 0, @@ -234,6 +248,13 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[] = { static const struct drm_i915_cmd_descriptor blt_cmds[] = { CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ), + CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x1FF, B, + .bits = {{ + .offset = 0, + .mask = MI_GLOBAL_GTT, + .expected = 0 + }}, + .bits_count = 1 ), CMD( MI_FLUSH_DW, SMI, !F, 0x3F, B, .bits = {{ .offset = 0,