On 19.08.2017 14:32, Mikko Perttunen wrote: > > > On 08/19/2017 02:11 PM, Dmitry Osipenko wrote: >> On 19.08.2017 13:35, Mikko Perttunen wrote: >>> On 08/19/2017 01:09 PM, Dmitry Osipenko wrote: >>>> On 19.08.2017 11:10, Mikko Perttunen wrote: >>>> [snip] >>>>>>> + host1x_hw_syncpt_set_protection(host, true); >>>>>> >>>>>> Is it really okay to force the protection? Maybe protection should be enabled >>>>>> with a respect to CONFIG_TEGRA_HOST1X_FIREWALL? In that case we would have to >>>>>> avoid software jobs validation for Tegra124+. >>>>> >>>>> I don't quite get your comment. The hardware syncpt protection layer being >>>>> enabled should never hurt - it doesn't mess with any valid jobs. It's also >>>>> only >>>>> on Tegra186 so I'm not sure where the Tegra124 comes from. >>>> >>>> Right, it's the gather filter on T124+, my bad. This raises several questions. >>>> >>>> 1) Why we have CONFIG_TEGRA_HOST1X_FIREWALL? Should it be always enforced or we >>>> actually want to be a bit more flexible and allow to disable it. Imagine that >>>> you are making a custom application and want to utilize channels in a >>>> different way. >>> >>> I think it should be up to the user to decide whether they want the firewall or >>> not. It's clearly the most useful on the older chips - especially Tegra20 due to >>> lack of IOMMU. The performance penalty is too great to force it on always. >>> >> >> Of course there is some overhead but is not that great. Usually command buffer >> contains just a dozen of commands. It should be an interesting challenge to >> optimize its performance though. >> >>> The programming model should always be considered the same - the rules of what >>> you are allowed to do are the same whether the firewall, or any >>> hardware-implemented protection features, are on or not. >>> >> >> Well, okay. >> >>>> >>>> 2) Since syncpoint protection is a T186 feature, what about previous >>>> generations? Should we validate syncpoints in software for them? We have >>>> 'syncpoint validation' patch staged in grate's kernel >>>> https://github.com/grate-driver/linux/commit/c8b6c82173f2ee9fead23380e8330b8099e7d5e7 >>>> >>>> >>>> (I'll start sending out this and other patches after a bit more thorough >>>> testing.) Improperly used syncpoints potentially could allow one program to >>>> damage others. >>> >>> Yes, I think the firewall should have this feature for older generations. We >>> could disable the check on Tegra186, as you point towards in question 4. >>> >>>> >>>> 3) What exactly does gather filter? Could you list all the commands that it >>>> filters out, please? >>> >>> According to the Tegra186 TRM (section 16.8.32), SETCLASS, SETSTRMID and EXTEND >>> are filtered. >>> >> >> Okay, then what about SETSTRMID command, I don't see its disassembly in the >> host1x gather debug dump. Is it accidentally missed? >> > > True, it's a new command in Tegra186 and I missed adding it to the disassembler. > It's probably fine to add it in another patch since it's only intended for > kernel use and it's useless without IOMMU support anyway (which we don't have > currently on Tegra186). > Yeah, but it probably would be more preferable that this patch would predate the "gather filter" enabling. >>>> >>>> 4) What about T30/T114 that do not have gather filter? Should we validate those >>>> commands for them in a software firewall? >>> >>> Yes, the firewall should validate that. >>> >>>> >>>> So maybe we should implement several layers of validation in the SW firewall. >>>> Like all layers for T20 (memory boundaries validation etc), software gather >>>> filter for T30/114 and software syncpoint validation for T30/114/124/210. >>>> >>> >>> That seems like a good idea. >> >> Alright, factoring out firewall from job.c probably should be the first step. >> -- Dmitry _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
