2023-04-13 08:54:45

by Ivan T. Ivanov

[permalink] [raw]
Subject: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

From: Tim Gover <[email protected]>

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 <[email protected]>
Signed-off-by: Ivan T. Ivanov <[email protected]>
---
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";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x0 0x0 0x0>;
+ no-map;
+ status = "disabled";
+ };
};

&v3d {
--
2.35.3


2023-04-13 16:30:12

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

Hi Ivan,

Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
> From: Tim Gover <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Ivan T. Ivanov <[email protected]>
> ---
> 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 {

2023-04-13 18:24:20

by Ivan T. Ivanov

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

On 04-13 18:15, Stefan Wahren wrote:
>
> Hi Ivan,
>
> Am 13.04.23 um 10:52 schrieb Ivan T. Ivanov:
> > From: Tim Gover <[email protected]>
> >
> > 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

2023-04-13 18:47:05

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

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 <[email protected]>
>>>
>>> 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
>

2023-04-13 19:42:44

by Tim Gover

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

On Thu, 13 Apr 2023 at 19:44, Stefan Wahren <[email protected]> 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 <[email protected]>
> >>>
> >>> 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

2023-04-16 13:14:17

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

Hi,

Am 13.04.23 um 21:28 schrieb Tim Gover:
> On Thu, 13 Apr 2023 at 19:44, Stefan Wahren <[email protected]> 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 <[email protected]>
>>>>>
>>>>> 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

2023-04-18 08:53:56

by Ivan T. Ivanov

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ARM: dts: Add nvmem node for BCM2711 bootloader public key

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