diff mbox series

[v2,2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

Message ID 20230413085206.149730-3-iivanov@suse.de
State New
Headers
Series nvmem: rmem: Make reserved region name unique |

Commit Message

Ivan T. Ivanov April 13, 2023, 8:52 a.m. UTC 
  From: Tim Gover <tim.gover@raspberrypi.com>

Make a copy of the bootloader secure-boot public key available to the OS
via an nvmem node. The placement information is populated by the
Raspberry Pi firmware if a public key is present in the BCM2711
bootloader EEPROM.

Signed-off-by: Tim Gover <tim.gover@raspberrypi.com>
Signed-off-by: Ivan T. Ivanov <iivanov@suse.de>
---
 arch/arm/boot/dts/bcm2711-rpi.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Stefan Wahren April 13, 2023, 4:15 p.m. UTC | #1 
Hi Ivan,

Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
> From: Tim Gover <tim.gover@raspberrypi.com>
> 
> Make a copy of the bootloader secure-boot public key available to the OS
> via an nvmem node. The placement information is populated by the
> Raspberry Pi firmware if a public key is present in the BCM2711
> bootloader EEPROM.

It would be nice to have a helpful link like:
https://www.raspberrypi.com/documentation/computers/configuration.html#nvmem-nodes

> 
> Signed-off-by: Tim Gover <tim.gover@raspberrypi.com>
> Signed-off-by: Ivan T. Ivanov <iivanov@suse.de>
> ---
>   arch/arm/boot/dts/bcm2711-rpi.dtsi | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/bcm2711-rpi.dtsi b/arch/arm/boot/dts/bcm2711-rpi.dtsi
> index 98817a6675b9..e30fbe84f9c3 100644
> --- a/arch/arm/boot/dts/bcm2711-rpi.dtsi
> +++ b/arch/arm/boot/dts/bcm2711-rpi.dtsi
> @@ -15,6 +15,7 @@ aliases {
>   		ethernet0 = &genet;
>   		pcie0 = &pcie0;
>   		blconfig = &blconfig;
> +		blpubkey = &blpubkey;
>   	};
>   };
>   
> @@ -67,6 +68,19 @@ blconfig: nvram@0 {
>   		no-map;
>   		status = "disabled";
>   	};
> +
> +	/*
> +	 * RPi4 will copy the binary public key blob (if present) from the bootloader
> +	 * into memory for use by the OS.
> +	 */
> +	blpubkey: nvram@1 {
> +		compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";

Yes this looks better, but this introduce a new dtbs_check issue. The 
new compatible must be documented in 
Documentation/devicetree/bindings/nvmem/rmem.yaml in a separate patch 
and reviewed by the DT guys.

Btw better use linux-arm-kernel list instead of lkml

Best regards

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x0 0x0 0x0>;
> +		no-map;
> +		status = "disabled";
> +	};
>   };
>   
>   &v3d {
Ivan T. Ivanov April 13, 2023, 6:18 p.m. UTC | #2 
On 04-13 18:15, Stefan Wahren wrote:
> 
> Hi Ivan,
> 
> Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
> > From: Tim Gover <tim.gover@raspberrypi.com>
> > 
> > Make a copy of the bootloader secure-boot public key available to the OS
> > via an nvmem node. The placement information is populated by the
> > Raspberry Pi firmware if a public key is present in the BCM2711
> > bootloader EEPROM.
> 
> It would be nice to have a helpful link like:
> https://www.raspberrypi.com/documentation/computers/configuration.html#nvmem-nodes

Yep, make sense.

> > +
> > +	/*
> > +	 * RPi4 will copy the binary public key blob (if present) from the bootloader
> > +	 * into memory for use by the OS.
> > +	 */
> > +	blpubkey: nvram@1 {
> > +		compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";
> 
> Yes this looks better, but this introduce a new dtbs_check issue. The new

Oops, yes, I forgot to make this check.

> compatible must be documented in
> Documentation/devicetree/bindings/nvmem/rmem.yaml in a separate patch and
> reviewed by the DT guys.

Or I can drop the new compatible string altogether? It looks like
only alias is strictly required?! Tim Gover is this correct?

Regards,
Ivan
Stefan Wahren April 13, 2023, 6:44 p.m. UTC | #3 
Hi Ivan,

Am 13.04.23 um 20:18 schrieb Ivan T. Ivanov:
> On 04-13 18:15, Stefan Wahren wrote:
>>
>> Hi Ivan,
>>
>> Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
>>> From: Tim Gover <tim.gover@raspberrypi.com>
>>>
>>> Make a copy of the bootloader secure-boot public key available to the OS
>>> via an nvmem node. The placement information is populated by the
>>> Raspberry Pi firmware if a public key is present in the BCM2711
>>> bootloader EEPROM.
>>
>> It would be nice to have a helpful link like:
>> https://www.raspberrypi.com/documentation/computers/configuration.html#nvmem-nodes
> 
> Yep, make sense.
> 
>>> +
>>> +	/*
>>> +	 * RPi4 will copy the binary public key blob (if present) from the bootloader
>>> +	 * into memory for use by the OS.
>>> +	 */
>>> +	blpubkey: nvram@1 {
>>> +		compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";
>>
>> Yes this looks better, but this introduce a new dtbs_check issue. The new
> 
> Oops, yes, I forgot to make this check.
> 
>> compatible must be documented in
>> Documentation/devicetree/bindings/nvmem/rmem.yaml in a separate patch and
>> reviewed by the DT guys.
> 
> Or I can drop the new compatible string altogether? It looks like
> only alias is strictly required?! Tim Gover is this correct?

i cannot speak for the firmware side, but i think we should try to keep 
it compatible with the vendor DTB here.

> 
> Regards,
> Ivan
>
Tim Gover April 13, 2023, 7:28 p.m. UTC | #4 
On Thu, 13 Apr 2023 at 19:44, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi Ivan,
>
> Am 13.04.23 um 20:18 schrieb Ivan T. Ivanov:
> > On 04-13 18:15, Stefan Wahren wrote:
> >>
> >> Hi Ivan,
> >>
> >> Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
> >>> From: Tim Gover <tim.gover@raspberrypi.com>
> >>>
> >>> Make a copy of the bootloader secure-boot public key available to the OS
> >>> via an nvmem node. The placement information is populated by the
> >>> Raspberry Pi firmware if a public key is present in the BCM2711
> >>> bootloader EEPROM.
> >>
> >> It would be nice to have a helpful link like:
> >> https://www.raspberrypi.com/documentation/computers/configuration.html#nvmem-nodes
> >
> > Yep, make sense.
> >
> >>> +
> >>> +   /*
> >>> +    * RPi4 will copy the binary public key blob (if present) from the bootloader
> >>> +    * into memory for use by the OS.
> >>> +    */
> >>> +   blpubkey: nvram@1 {
> >>> +           compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";
> >>
> >> Yes this looks better, but this introduce a new dtbs_check issue. The new
> >
> > Oops, yes, I forgot to make this check.
> >
> >> compatible must be documented in
> >> Documentation/devicetree/bindings/nvmem/rmem.yaml in a separate patch and
> >> reviewed by the DT guys.
> >
> > Or I can drop the new compatible string altogether? It looks like
> > only alias is strictly required?! Tim Gover is this correct?
>
> i cannot speak for the firmware side, but i think we should try to keep
> it compatible with the vendor DTB here.
>

The firmware doesn't look at the compatible string. It locates the
nodes to update using the 'blconfig' and 'blpubkey' aliases. Userspace
scripts (including the documentation example) should also use these
aliases.
Therefore, I don't think it matters if the compatible strings is
modified, but I won't pretend to know what the correct DT style is
here :)

Tim
Stefan Wahren April 16, 2023, 1:11 p.m. UTC | #5 
Hi,

Am 13.04.23 um 21:28 schrieb Tim Gover:
> On Thu, 13 Apr 2023 at 19:44, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Hi Ivan,
>>
>> Am 13.04.23 um 20:18 schrieb Ivan T. Ivanov:
>>> On 04-13 18:15, Stefan Wahren wrote:
>>>>
>>>> Hi Ivan,
>>>>
>>>> Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
>>>>> From: Tim Gover <tim.gover@raspberrypi.com>
>>>>>
>>>>> Make a copy of the bootloader secure-boot public key available to the OS
>>>>> via an nvmem node. The placement information is populated by the
>>>>> Raspberry Pi firmware if a public key is present in the BCM2711
>>>>> bootloader EEPROM.
>>>>
>>>> It would be nice to have a helpful link like:
>>>> https://www.raspberrypi.com/documentation/computers/configuration.html#nvmem-nodes
>>>
>>> Yep, make sense.
>>>
>>>>> +
>>>>> +   /*
>>>>> +    * RPi4 will copy the binary public key blob (if present) from the bootloader
>>>>> +    * into memory for use by the OS.
>>>>> +    */
>>>>> +   blpubkey: nvram@1 {
>>>>> +           compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";
>>>>
>>>> Yes this looks better, but this introduce a new dtbs_check issue. The new
>>>
>>> Oops, yes, I forgot to make this check.
>>>
>>>> compatible must be documented in
>>>> Documentation/devicetree/bindings/nvmem/rmem.yaml in a separate patch and
>>>> reviewed by the DT guys.
>>>
>>> Or I can drop the new compatible string altogether? It looks like
>>> only alias is strictly required?! Tim Gover is this correct?
>>
>> i cannot speak for the firmware side, but i think we should try to keep
>> it compatible with the vendor DTB here.
>>
> 
> The firmware doesn't look at the compatible string. It locates the
> nodes to update using the 'blconfig' and 'blpubkey' aliases. Userspace
> scripts (including the documentation example) should also use these
> aliases.
> Therefore, I don't think it matters if the compatible strings is
> modified, but I won't pretend to know what the correct DT style is
> here :)

okay, regardless of the compatible string the patch must be send to the 
DT maintainers and the devicetree mailing list otherwise they don't have 
any chance to review.

Thanks

> 
> Tim
Ivan T. Ivanov April 18, 2023, 8:49 a.m. UTC | #6 
Hi,

On 04-16 15:11, Stefan Wahren wrote:
> > > > 
> > > > Or I can drop the new compatible string altogether? It looks like
> > > > only alias is strictly required?! Tim Gover is this correct?
> > > 
> > > i cannot speak for the firmware side, but i think we should try to keep
> > > it compatible with the vendor DTB here.
> > > 
> > 
> > The firmware doesn't look at the compatible string. It locates the
> > nodes to update using the 'blconfig' and 'blpubkey' aliases. Userspace
> > scripts (including the documentation example) should also use these
> > aliases.
> > Therefore, I don't think it matters if the compatible strings is
> > modified, but I won't pretend to know what the correct DT style is
> > here :)

Ok. Perhaps Stefan have a point and will be better if we keep things 
in sync between vendor DTS and upstream one.

> 
> okay, regardless of the compatible string the patch must be send to the DT
> maintainers and the devicetree mailing list otherwise they don't have any
> chance to review.
> 

Sure, my fault. I just used list of recipients from the initial patch.  

Regards,
Ivan
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/bcm2711-rpi.dtsi b/arch/arm/boot/dts/bcm2711-rpi.dtsi
index 98817a6675b9..e30fbe84f9c3 100644
--- a/arch/arm/boot/dts/bcm2711-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2711-rpi.dtsi
@@ -15,6 +15,7 @@  aliases {
 		ethernet0 = &genet;
 		pcie0 = &pcie0;
 		blconfig = &blconfig;
+		blpubkey = &blpubkey;
 	};
 };
 
@@ -67,6 +68,19 @@  blconfig: nvram@0 {
 		no-map;
 		status = "disabled";
 	};
+
+	/*
+	 * RPi4 will copy the binary public key blob (if present) from the bootloader
+	 * into memory for use by the OS.
+	 */
+	blpubkey: nvram@1 {
+		compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x0 0x0 0x0>;
+		no-map;
+		status = "disabled";
+	};
 };
 
 &v3d {