From:   Horia Geanta <horia.geanta@nxp.com>
To:     Chris Spencer <spencercw@gmail.com>
CC:     Aymen Sghaier <aymen.sghaier@nxp.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        Chris Spencer <christopher.spencer@sea.co.uk>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [RFC 4/4] crypto: caam - use job ring for RNG instantiation
 instead of DECO
Thread-Topic: [RFC 4/4] crypto: caam - use job ring for RNG instantiation
 instead of DECO
Thread-Index: AQHUypZsrSD4izM7BUuBqlVaxZEyMQ==
Date:   Tue, 26 Feb 2019 14:53:42 +0000
Message-ID: <VI1PR0402MB34857504705A898A59085B4B987B0@VI1PR0402MB3485.eurprd04.prod.outlook.com>
References: <20190222100613.2290-1-christopher.spencer@sea.co.uk>
 <20190222100613.2290-5-christopher.spencer@sea.co.uk>
 <VI1PR0402MB34851F1E7021F16B45F74E55987A0@VI1PR0402MB3485.eurprd04.prod.outlook.com>
 <CAGUNXRTvXVU7vRicF=mR7xxCj=p+Sqj3aNnL2TezLZXhHFkvzA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 23b49d38-d59e-445b-737b-08d69bfa338f
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Feb 2019 14:53:42.7151
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3501
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 2/25/2019 4:37 PM, Chris Spencer wrote:=0A=
> On Mon, 25 Feb 2019 at 14:22, Horia Geanta <horia.geanta@nxp.com> wrote:=
=0A=
>>=0A=
>> On 2/22/2019 12:07 PM, spencercw@gmail.com wrote:=0A=
>>> From: Chris Spencer <christopher.spencer@sea.co.uk>=0A=
>>>=0A=
>>> This is required to support the i.MX8.=0A=
>>>=0A=
>> Why exactly is this required?=0A=
>> You should provide more details.=0A=
> =0A=
> I don't know. :) These changes were made in [1], although the change=0A=
> is actually described in [2] where it says: "Use a Job ring for RNG=0A=
> instantiation rather than DECO, even for i.MX6/7 families." This does=0A=
> indeed seem to be necessary to get it to work, but I can only=0A=
> speculate why.=0A=
> =0A=
The commit messages you refer are indeed rather poor, however you need to=
=0A=
understand the context: the history reflects NXP internal development.=0A=
=0A=
It's expected for the changes to be clearly explained, justified when reach=
ing=0A=
mainline kernel.=0A=
=0A=
> The original commit which added the run_descriptor_deco0 function [3]=0A=
> states "[...] and another function that performs the running of the=0A=
> said descriptor using the DECO debug mechanism." My best guess is that=0A=
> the DECO debug mechanism is no longer available so it has to go via=0A=
> the job ring.=0A=
> =0A=
This change is actually orthogonal to i.MX8 MQ support and is related to=0A=
SECO-based i.MX8 SoCs (QXP, QM) and/or OP-TEE support.=0A=
=0A=
Access to CAAM global registers (CCSR page 0) won't be allowed in cases whe=
n=0A=
Linux runs in Normal World.=0A=
From this perspective, it makes sense to eliminate accesses to these regist=
ers.=0A=
Some of the accesses are done for RNG initialization via the DECO debug=0A=
mechanism; to remove them, RNG is initialized via the Job Ring interface.=
=0A=
However, I still see some illegal accesses - for e.g. reading RDSTA registe=
r to=0A=
check which virtual RNGs ("state handles" in RM terms) are already initiali=
zed.=0A=
=0A=
It looks like cutting access to CAAM global registers must come with some=
=0A=
assumptions (since it implies some checks can no longer be performed, see R=
DSTA=0A=
above).=0A=
For e.g. in case OP-TEE is detected, we could assume that RNG was already=
=0A=
initialized. Similar for other cases like SECO f/w (i.MX8 QM and QXP).=0A=
If we make this assumption, it actually means RNG initialization is no long=
er=0A=
needed:=0A=
=0A=
if (!ctrlpriv->mc_en && rng_vid >=3D 4 && !normal_world) {=0A=
	... // rng init as before - using DECO debug mechanism=0A=
}=0A=
=0A=
>> Strictly related to the implementation, why is the code moved to a new f=
ile?=0A=
>> Please do not shuffle the code without providing a good reason.=0A=
> =0A=
> This was done in an attempt to keep it reasonably close to the changes=0A=
> in the downstream driver. The file was moved in [1] with the=0A=
> description "Move the code related to RNG instanciation to another=0A=
> file to ease comprehension."=0A=
> =0A=
> The RNG initialisation code is not really needed in ctrl.c anymore=0A=
> since it is only used in jr.c, so moving it out seems sensible.=0A=
> =0A=
> Thanks,=0A=
> Chris=0A=
> =0A=
> [1] https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/c=
rypto/caam/inst_rng.c?h=3Dimx_4.14.78_1.0.0_ga&id=3D5fd8fe866172c38e8cd0774=
1d98d79b995f05bc9=0A=
> [2] https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/c=
rypto/caam/ctrl.c?h=3Dimx_4.14.78_1.0.0_ga&id=3Dafa53c2c96a54b4432a26372e3b=
5e7dbccbb9671=0A=
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/co=
mmit/drivers/crypto/caam/ctrl.c?id=3D04cddbfe6b5f334aa337a1a9797eb8914822f2=
f8=0A=
> =0A=