linux-kernel - Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

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] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 09:18:11 +0100
From: Conor Dooley <conor.dooley@...rochip.com>
To: Conor Dooley <conor@...nel.org>
CC: Viacheslav <adeep@...ina.in>, Rob Herring <robh@...nel.org>, Neil
 Armstrong <neil.armstrong@...aro.org>, Kevin Hilman <khilman@...libre.com>,
	Jerome Brunet <jbrunet@...libre.com>, Martin Blumenstingl
	<martin.blumenstingl@...glemail.com>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-amlogic@...ts.infradead.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	<devicetree@...r.kernel.org>
Subject: Re: [PATCH v5 3/4] dt-bindings: arm: amlogic:
 amlogic,meson-gx-ao-secure: add secure-monitor property

On Mon, Jun 17, 2024 at 05:57:53PM +0100, Conor Dooley wrote:
> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
> > Thanks for review.
> > 
> > 13/06/2024 19.42, Rob Herring wrote:
> > > On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
> > > > On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
> > > > > Hi!
> > > > > 
> > > > > 10/06/2024 19.08, Conor Dooley wrote:
> > > > > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
> > > > > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
> > > > > > 
> > > > > > "bindings are for hardware, not drivers". Why purpose does the "secure
> > > > > > monitor" serve that the secure firmware needs a reference to it?
> > > > > 
> > > > > This driver is an extension to the meson-gx-socinfo driver: it supplements
> > > > > information obtained from the register with information from the
> > > > > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
> > > > > loading order, we cannot do away with meson-gx-socinfo, as it is used for
> > > > > platform identification in some drivers. Therefore, the extended information
> > > > > is formatted as a separate driver, which is loaded after the secure-monitor
> > > > > driver.
> > > > 
> > > > Please stop talking about drivers, this is a binding which is about
> > > > hardware. Please provide, in your next version, a commit message that
> > > > justifies adding this property without talking about driver probing
> > > > order etc, and instead focuses on what service the "secure monitor"
> > > > provides etc.
> > > 
> > > To put it another way, how many secure monitors does 1 system have?
> > 
> > One per system in current device tree.
> 
> One per system, or one is currently described per system, but more might
> be added later?
> 
> > > What do you do if the property is not present? You didn't make it
> > > required which is good because that would be an ABI break.
> > 
> > We need an indication of the ability to use the secure-monitor to obtain
> > additional information within the soc driver. It seemed to me that using an
> > explicit reference to the secure-monitor is the best choice.
> > 
> > > 
> > > You only need a link in DT if there are different possible providers or
> > > some per consumer information to describe (e.g. an interrupt number or
> > > clock ID). You don't have the latter and likely there is only 1 possible
> > > provider.
> > 
> > Would replacing the reference to sm with an option, for example,
> > use-secure-monitor = <1>; look more appropriate in this case?
> 
> Perhaps a silly question, but (provided there's only one per system, why
> can't the secure-monitor driver expose a function that you can call to get
> a reference to the system-monitor? I did something similar before with
> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
> mpfs-rng is probed from software so it's slightly different to your
> case, but the principle is the same and it's not unheard of for code in
> drivers/soc to expose interfaces to other drivers like this. You can
> just call a function like that, and know whether there's a secure
> monitor, without having to retrofit a DT property.

Another thing, without having a driver expose an API, is calling
of_find_compatible_node() to find the node. That also doesn't require
retrofitting properties.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)