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).
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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
