linux-kernel - Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Apr 2018 13:56:55 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common
 tegra-mc driver

On 27.04.2018 13:24, Thierry Reding wrote:
> On Fri, Apr 27, 2018 at 01:13:47PM +0300, Dmitry Osipenko wrote:
>> On 27.04.2018 12:34, Thierry Reding wrote:
>>> On Mon, Apr 09, 2018 at 10:28:31PM +0300, Dmitry Osipenko wrote:
>>> [...]
>>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> [...]
>>>> +#define MC_GART_ERROR_REQ		0x30
>>>> +#define MC_DECERR_EMEM_OTHERS_STATUS	0x58
>>>> +#define MC_SECURITY_VIOLATION_STATUS	0x74
>>> [...]
>>>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>>> [...]
>>>> @@ -21,19 +21,30 @@
>>>>  #define MC_INT_INVALID_SMMU_PAGE (1 << 10)
>>>>  #define MC_INT_ARBITRATION_EMEM (1 << 9)
>>>>  #define MC_INT_SECURITY_VIOLATION (1 << 8)
>>>> +#define MC_INT_INVALID_GART_PAGE (1 << 7)
>>>>  #define MC_INT_DECERR_EMEM (1 << 6)
>>>>  
>>>>  static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
>>>>  {
>>>> +	if (mc->regs2 && offset >= 0x24)
>>>> +		return readl(mc->regs2 + offset - 0x3c);
>>>
>>> I'm still not sure how this is supposed to work. If we pass in
>>> MC_GART_ERROR_REQ as offset into mc_readl(), then the condition above
>>> will be true (0x30 >= 0x24) but then the new offset will be computed
>>> and we end up with:
>>>
>>> 	return readl(mc->regs2 + 0x30 - 0x3c);
>>>
>>> which means we'll be adding a negative offset (or rather a very large
>>> offset because it will wrap around).
>>
>> Indeed! Thank you for pointing at it again, now I see the issue. It probably
>> works because actual registers mapping is aligned to page(?) size and adding the
>> large offset with wraparound is equal to subtraction.
>>
>> That register belongs to the GART and we can't simply move interrupt handling to
>> the GART driver because status register is within the MC in device tree. We can
>> omit reading of MC_GART_ERROR_REQ and simply report GART page fault for the
>> starter and then reorganize drivers by making MC driver MFD and GART its
>> sub-device, what do you think?
> 
> Sounds like a good idea. Can you send a fix on top of this that I can
> squash into this when applying?

Sure.

> As for integrating GART with MC, I'd prefer something that doesn't use
> MFD but rather does something similar to what we have for the SMMU. I
> think that's simpler to do and has less boilerplate. I think it's also
> warranted because the MC and GART are very tightly coupled, so an MFD
> would be slightly over-engineered, in my opinion.

Okay, I'll recap how SMMU is integrated with MC and then come up with something.