On Tue, Feb 04, 2014 at 10:45:45AM -0800, Volkin, Bradley D wrote: > The current table structure is that we have tables per-ring and per-gen (plus the table > for common MI commands) and all tables are treated as blacklist/greylist. The proposed > flow here would indicate that we need tables per-ring, per-client, per-gen and that some > would be treated as a whitelist and some as a blacklist/greylist. > > I think the benefit to these changes amounts to preventing clients from issuing invalid > MI commands, but clients can do that today, so it's not a regression right? It could also > make it easier to safely cover new platforms if MI commands were added, though the parser > is strictly limited to gen7 today and would need additional work to enable it for new gens. The benefit is in enabling future platforms - with an explicit MI whitelist we'd forced to re-audit MI_ commands fully and there's no chance to miss something. Also the MI_ commands are the tricky ones, so if one slips through we have a problem. Relying on the command streamer hw to catch invalid opcodes is something we need to, so no benefit for that really. > The set of MI commands for current gens is small compared to the other command ranges. > On the one hand, that makes it easier to create a whitelist of the commands. On the other > hand, it also makes it easy to just audit the commands in the spec and validate that the > blacklist/greylist covers the potential issues. Since we only care about the set of allowed MI commands the list probably even shrinks a bit. > So, if we maintain the conservative parser enabling and re-audit the lists when enabling > new gens, and if we can live with clients issuing totally invalid commands (and I get the > feeling that we have to), then I think the current solution gets us the benefits we want > with less complexity. > > Let me know what you think. I'll continue working through the other feedback either way. I didn't really consider that the MI whitelist would have a bigger impact on the code really. So if you expect this change to cause a bit a delay in getting the next round ready then we can postpone this a bit - for me the important part is to get the parser in soonish so that we can start to catch regressions (if there are any we haven't considered yet). But longer-term I think switching to a whiteliste for MI commands is the right approach. > > Iirc we need a special-cases list for blitter commands anyway due to their > > irregular lenght, so maybe we could do a full whitelist for that one, too. > > All rings have commands with irregular length encodings. I believe they also all have > commands with non-fixed lengths (e.g. XY_TEXT_IMMEDIATE_BLT on blt, MEDIA_OBJECT on render). Yeah, variable length commands are everywhere, but I've thought commands which don't use the usual lenght-2 encoding (i.e. those which are just 1 dword) are restricted to MI commands and the blitter. Am I mistaken on this? Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
