[PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Erik Faye-Lund]

On Mon, Oct 7, 2013 at 2:14 PM, Thierry Reding <thierry.reding at gmail.com> wrote:
> On Mon, Oct 07, 2013 at 01:34:44PM +0200, Erik Faye-Lund wrote:
>> On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
>> <thierry.reding at gmail.com> wrote:
>> > Rework the address table code for the host1x firewall. The previous
>> > implementation allocated a bitfield but didn't check for a valid pointer
>> > so it could potentially crash.
>>
>> I don't think it could crash. The bitmaps was allocated as a 256-bit
>> field, and the register offset gets AND'ed with 0xFF before being
>> looked up. What am I missing?
>
> The pointer returned from the allocation is never checked, so it could
> theoretically be NULL and be used regardless.
>

Right. Thanks for clarifying.

> Also I'm not sure that AND'ing with 0xff is the right thing to do here.

Well, maybe not. I'm not entirely convinced it's *not*, though.

>> > Furthermore setting up a bitfield makes
>> > the code more complex than it needs to be.
>>
>> Doesn't this perform worse than the current implementation? Going from
>> 1 to 13 checks per write sounds less than ideal to me...
>
> I'm not so sure. Caching should alleviate the issue with the increased
> amount of data. Then there's the fact that previously we needed to
> divide and compute the remainder of the division for the BIT_MASK and
> BIT_WORD operations.

That's an AND and a shift, not division and modulo, both single-cycle
instructions on ARM. I'm pretty sure that'd be a win.

> One other added benefit of this approach is that the address registers
> are stored in a const array and therefore could reside in a read-only
> memory region. With the previous approach, once somebody had access to
> the bitmap, it could easily be overwritten with zeros and effectively
> make the firewall useless. I'm not sure how likely that would be, but it
> could be done.

Perhaps the bitmap could be generated compile-time and stuck in
read-only memory?

> I guess one could actually go and run a benchmark against both versions
> and balance the performance impact against the possible security
> implications. But given that we don't really have any benchmarks that's
> pretty hard to do.

Well, perhaps we could have both? :)