[PATCH 0/1] Add IMR driver for Keem Bay

[PATCH 0/1] Add IMR driver for Keem Bay

[Daniele Alessandrelli]

The following is a patch for a new Intel Movidius SoC, code-named Keem
Bay.

Keem Bay needs a driver to disable the Isolated Memory Region (IMR)
set up by the SoC bootloader during early boot.

If such an IMR is not disabled and some device tries to access it,
the system will reboot.

Since this driver is SoC-specific and Keem Bay is a new SoC, I was
unsure of where to put this driver. In the end I decided to create a
new 'keembay' directory in 'drivers/soc'. I hope that's reasonable, if
not, just let me know.


Daniele Alessandrelli (1):
  soc: keembay: Add Keem Bay IMR driver

 MAINTAINERS                       |  5 ++++
 drivers/soc/Kconfig               |  1 +
 drivers/soc/Makefile              |  1 +
 drivers/soc/keembay/Kconfig       | 22 +++++++++++++++++
 drivers/soc/keembay/Makefile      |  5 ++++
 drivers/soc/keembay/keembay-imr.c | 40 +++++++++++++++++++++++++++++++
 6 files changed, 74 insertions(+)
 create mode 100644 drivers/soc/keembay/Kconfig
 create mode 100644 drivers/soc/keembay/Makefile
 create mode 100644 drivers/soc/keembay/keembay-imr.c

-- 
2.21.1

[PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Daniele Alessandrelli]

From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>

Keem Bay bootloader sets up a temporary Isolated Memory Region (IMR) to
protect itself during pre-Linux boot.

This temporary IMR remains active even when control is passed to the
Linux Kernel. It is Kernel responsibility to remove such an IMR during
initialization.

This driver adds such functionality.

The driver is loaded during `early_init`, which should ensure that the
IMR is removed before devices that may try to access the IMR are
initialized.

Signed-off-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
---
 MAINTAINERS                       |  5 ++++
 drivers/soc/Kconfig               |  1 +
 drivers/soc/Makefile              |  1 +
 drivers/soc/keembay/Kconfig       | 22 +++++++++++++++++
 drivers/soc/keembay/Makefile      |  5 ++++
 drivers/soc/keembay/keembay-imr.c | 40 +++++++++++++++++++++++++++++++
 6 files changed, 74 insertions(+)
 create mode 100644 drivers/soc/keembay/Kconfig
 create mode 100644 drivers/soc/keembay/Makefile
 create mode 100644 drivers/soc/keembay/keembay-imr.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b816a453b10e..59f1923a0f25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9194,6 +9194,11 @@ S:	Maintained
 W:	http://lse.sourceforge.net/kdump/
 F:	Documentation/admin-guide/kdump/
 
+KEEMBAY IMR
+M:	Daniele Alessandrelli <daniele.alessandrelli@intel.com>
+S:	Maintained
+F:	drivers/soc/keembay/keembay-imr.c
+
 KEENE FM RADIO TRANSMITTER DRIVER
 M:	Hans Verkuil <hverkuil@xs4all.nl>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 425ab6f7e375..eeeba3ef7338 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
 source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/imx/Kconfig"
 source "drivers/soc/ixp4xx/Kconfig"
+source "drivers/soc/keembay/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/renesas/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 36452bed86ef..65c981207283 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -13,6 +13,7 @@ obj-y				+= fsl/
 obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
 obj-y				+= imx/
 obj-$(CONFIG_ARCH_IXP4XX)	+= ixp4xx/
+obj-y				+= keembay/
 obj-$(CONFIG_SOC_XWAY)		+= lantiq/
 obj-y				+= mediatek/
 obj-y				+= amlogic/
diff --git a/drivers/soc/keembay/Kconfig b/drivers/soc/keembay/Kconfig
new file mode 100644
index 000000000000..2161bce131b3
--- /dev/null
+++ b/drivers/soc/keembay/Kconfig
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Keem Bay SoC drivers.
+#
+
+menu "Keem Bay SoC drivers"
+
+config KEEMBAY_IMR
+	bool "Clean-up Keem Bay bootloader IMR at boot"
+	depends on ARM64
+	help
+	  This option makes the Kernel clean up the Isolated Memory Region
+	  (IMR) set up by Keem Bay bootloader (U-boot) to protect itself during
+	  early boot.
+
+	  The IMR number to be cleaned up is taken from the device tree
+	  (property 'u-boot-imr' of the 'chosen' node).
+
+	  If you are compiling the Kernel for a Keem Bay SoC select Y,
+	  otherwise select N.
+
+endmenu
diff --git a/drivers/soc/keembay/Makefile b/drivers/soc/keembay/Makefile
new file mode 100644
index 000000000000..dacfdb9f5fc1
--- /dev/null
+++ b/drivers/soc/keembay/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for Keem Bay SoC drivers.
+#
+obj-$(CONFIG_KEEMBAY_IMR) += keembay-imr.o
diff --git a/drivers/soc/keembay/keembay-imr.c b/drivers/soc/keembay/keembay-imr.c
new file mode 100644
index 000000000000..eabbdd6e69a7
--- /dev/null
+++ b/drivers/soc/keembay/keembay-imr.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019-2020 Intel Corporation
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/arm-smccc.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/printk.h>
+#include <linux/types.h>
+
+/* Keem Bay SiP SVC for clearing an IMR. */
+#define KMB_SIP_SVC_IMR_CLEAR	0x8200ff13
+
+static int __init clear_imr(u64 imr)
+{
+	struct arm_smccc_res res = { 0 };
+
+	arm_smccc_smc(KMB_SIP_SVC_IMR_CLEAR, imr, 0, 0, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+static int __init kmb_imr_init(void)
+{
+	u32 imr;
+	int rc;
+
+	rc = of_property_read_u32(of_chosen, "u-boot-imr", &imr);
+	if (rc) {
+		pr_warn("Skipping IMR clean-up: No U-Boot IMR defined in device tree\n");
+		return 0;
+	}
+	pr_info("Disabling Keem Bay U-boot IMR: %u\n", imr);
+
+	return clear_imr(imr);
+}
+early_initcall(kmb_imr_init);
-- 
2.21.1

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[Alessandrelli, Daniele]

On Tue, 2020-04-21 at 17:36 +0100, Daniele Alessandrelli wrote:
> The following is a patch for a new Intel Movidius SoC, code-named
> Keem
> Bay.
> 
> Keem Bay needs a driver to disable the Isolated Memory Region (IMR)
> set up by the SoC bootloader during early boot.
> 
> If such an IMR is not disabled and some device tries to access it,
> the system will reboot.
> 
> Since this driver is SoC-specific and Keem Bay is a new SoC, I was
> unsure of where to put this driver. In the end I decided to create a
> new 'keembay' directory in 'drivers/soc'. I hope that's reasonable,
> if
> not, just let me know.
> 
> 
> Daniele Alessandrelli (1):
>   soc: keembay: Add Keem Bay IMR driver
> 
>  MAINTAINERS                       |  5 ++++
>  drivers/soc/Kconfig               |  1 +
>  drivers/soc/Makefile              |  1 +
>  drivers/soc/keembay/Kconfig       | 22 +++++++++++++++++
>  drivers/soc/keembay/Makefile      |  5 ++++
>  drivers/soc/keembay/keembay-imr.c | 40
> +++++++++++++++++++++++++++++++
>  6 files changed, 74 insertions(+)
>  create mode 100644 drivers/soc/keembay/Kconfig
>  create mode 100644 drivers/soc/keembay/Makefile
>  create mode 100644 drivers/soc/keembay/keembay-imr.c
> 

Adding some more people (Arnd and linux-arm-kernel ML) in CC in the
hope of getting some guidance on how to have this patch merged.

I'm a novice, so I wonder if the lack of feedback is because I'm doing
something wrong or if I just sent the initial email to the wrong people
/ ML.

I'd appreciate any help you can provide.
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[Alessandrelli, Daniele]

On Tue, 2020-04-21 at 17:36 +0100, Daniele Alessandrelli wrote:
> The following is a patch for a new Intel Movidius SoC, code-named
> Keem
> Bay.
> 
> Keem Bay needs a driver to disable the Isolated Memory Region (IMR)
> set up by the SoC bootloader during early boot.
> 
> If such an IMR is not disabled and some device tries to access it,
> the system will reboot.
> 
> Since this driver is SoC-specific and Keem Bay is a new SoC, I was
> unsure of where to put this driver. In the end I decided to create a
> new 'keembay' directory in 'drivers/soc'. I hope that's reasonable,
> if
> not, just let me know.
> 
> 
> Daniele Alessandrelli (1):
>   soc: keembay: Add Keem Bay IMR driver
> 
>  MAINTAINERS                       |  5 ++++
>  drivers/soc/Kconfig               |  1 +
>  drivers/soc/Makefile              |  1 +
>  drivers/soc/keembay/Kconfig       | 22 +++++++++++++++++
>  drivers/soc/keembay/Makefile      |  5 ++++
>  drivers/soc/keembay/keembay-imr.c | 40
> +++++++++++++++++++++++++++++++
>  6 files changed, 74 insertions(+)
>  create mode 100644 drivers/soc/keembay/Kconfig
>  create mode 100644 drivers/soc/keembay/Makefile
>  create mode 100644 drivers/soc/keembay/keembay-imr.c
> 

Adding some more people (Arnd and linux-arm-kernel ML) in CC in the
hope of getting some guidance on how to have this patch merged.

I'm a novice, so I wonder if the lack of feedback is because I'm doing
something wrong or if I just sent the initial email to the wrong people
/ ML.

I'd appreciate any help you can provide.
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[gregkh]

On Thu, Apr 30, 2020 at 07:49:36PM +0000, Alessandrelli, Daniele wrote:
> This e-mail and any attachments may contain confidential material for the sole
> use of the intended recipient(s). Any review or distribution by others is
> strictly prohibited. If you are not the intended recipient, please contact the
> sender and delete all copies.

This footer means I delete all of your emails...

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[gregkh]

On Thu, Apr 30, 2020 at 07:49:36PM +0000, Alessandrelli, Daniele wrote:
> This e-mail and any attachments may contain confidential material for the sole
> use of the intended recipient(s). Any review or distribution by others is
> strictly prohibited. If you are not the intended recipient, please contact the
> sender and delete all copies.

This footer means I delete all of your emails...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[Daniele Alessandrelli]

On Fri, 2020-05-01 at 09:09 +0200, gregkh@linuxfoundation.org wrote:
> On Thu, Apr 30, 2020 at 07:49:36PM +0000, Alessandrelli, Daniele
> wrote:
> > This e-mail and any attachments may contain confidential material
> > for the sole
> > use of the intended recipient(s). Any review or distribution by
> > others is
> > strictly prohibited. If you are not the intended recipient, please
> > contact the
> > sender and delete all copies.
> 
> This footer means I delete all of your emails...

My bad, I replied using the wrong email address (i.e., the one that
adds the footer automatically). Sorry about that :/
I'll be more careful the next time.

The original emails (the ones with the cover letter and the patch) were
fine though (unless I did something else wrong). Any advice on how to
have the patch reviewed and eventually merged?

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[Daniele Alessandrelli]

On Fri, 2020-05-01 at 09:09 +0200, gregkh@linuxfoundation.org wrote:
> On Thu, Apr 30, 2020 at 07:49:36PM +0000, Alessandrelli, Daniele
> wrote:
> > This e-mail and any attachments may contain confidential material
> > for the sole
> > use of the intended recipient(s). Any review or distribution by
> > others is
> > strictly prohibited. If you are not the intended recipient, please
> > contact the
> > sender and delete all copies.
> 
> This footer means I delete all of your emails...

My bad, I replied using the wrong email address (i.e., the one that
adds the footer automatically). Sorry about that :/
I'll be more careful the next time.

The original emails (the ones with the cover letter and the patch) were
fine though (unless I did something else wrong). Any advice on how to
have the patch reviewed and eventually merged?








_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[gregkh]

On Fri, May 01, 2020 at 08:53:34AM +0100, Daniele Alessandrelli wrote:
> On Fri, 2020-05-01 at 09:09 +0200, gregkh@linuxfoundation.org wrote:
> > On Thu, Apr 30, 2020 at 07:49:36PM +0000, Alessandrelli, Daniele
> > wrote:
> > > This e-mail and any attachments may contain confidential material
> > > for the sole
> > > use of the intended recipient(s). Any review or distribution by
> > > others is
> > > strictly prohibited. If you are not the intended recipient, please
> > > contact the
> > > sender and delete all copies.
> > 
> > This footer means I delete all of your emails...
> 
> My bad, I replied using the wrong email address (i.e., the one that
> adds the footer automatically). Sorry about that :/
> I'll be more careful the next time.
> 
> The original emails (the ones with the cover letter and the patch) were
> fine though (unless I did something else wrong). Any advice on how to
> have the patch reviewed and eventually merged?
> 
> 
> 
> 
> 
> 
> 

Ok, let me go look at the code...

Re: [PATCH 0/1] Add IMR driver for Keem Bay

[gregkh]

On Fri, May 01, 2020 at 08:53:34AM +0100, Daniele Alessandrelli wrote:
> On Fri, 2020-05-01 at 09:09 +0200, gregkh@linuxfoundation.org wrote:
> > On Thu, Apr 30, 2020 at 07:49:36PM +0000, Alessandrelli, Daniele
> > wrote:
> > > This e-mail and any attachments may contain confidential material
> > > for the sole
> > > use of the intended recipient(s). Any review or distribution by
> > > others is
> > > strictly prohibited. If you are not the intended recipient, please
> > > contact the
> > > sender and delete all copies.
> > 
> > This footer means I delete all of your emails...
> 
> My bad, I replied using the wrong email address (i.e., the one that
> adds the footer automatically). Sorry about that :/
> I'll be more careful the next time.
> 
> The original emails (the ones with the cover letter and the patch) were
> fine though (unless I did something else wrong). Any advice on how to
> have the patch reviewed and eventually merged?
> 
> 
> 
> 
> 
> 
> 

Ok, let me go look at the code...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Greg Kroah-Hartman]

On Tue, Apr 21, 2020 at 05:36:18PM +0100, Daniele Alessandrelli wrote:
> From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> 
> Keem Bay bootloader sets up a temporary Isolated Memory Region (IMR) to
> protect itself during pre-Linux boot.
> 
> This temporary IMR remains active even when control is passed to the
> Linux Kernel. It is Kernel responsibility to remove such an IMR during
> initialization.
> 
> This driver adds such functionality.
> 
> The driver is loaded during `early_init`, which should ensure that the
> IMR is removed before devices that may try to access the IMR are
> initialized.
> 
> Signed-off-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>

First off, there is a "proper" way to send patches to the kernel
community that Intel has that I do not think you are following.  Please
work with the Intel "Linux group" to do that first, as odds are they
would have caught all of these issues beforehand (hint, which is why
that process is in place...)

> ---
>  MAINTAINERS                       |  5 ++++
>  drivers/soc/Kconfig               |  1 +
>  drivers/soc/Makefile              |  1 +
>  drivers/soc/keembay/Kconfig       | 22 +++++++++++++++++
>  drivers/soc/keembay/Makefile      |  5 ++++
>  drivers/soc/keembay/keembay-imr.c | 40 +++++++++++++++++++++++++++++++
>  6 files changed, 74 insertions(+)
>  create mode 100644 drivers/soc/keembay/Kconfig
>  create mode 100644 drivers/soc/keembay/Makefile
>  create mode 100644 drivers/soc/keembay/keembay-imr.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b816a453b10e..59f1923a0f25 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9194,6 +9194,11 @@ S:	Maintained
>  W:	http://lse.sourceforge.net/kdump/
>  F:	Documentation/admin-guide/kdump/
>  
> +KEEMBAY IMR
> +M:	Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> +S:	Maintained
> +F:	drivers/soc/keembay/keembay-imr.c
> +
>  KEENE FM RADIO TRANSMITTER DRIVER
>  M:	Hans Verkuil <hverkuil@xs4all.nl>
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 425ab6f7e375..eeeba3ef7338 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
> +source "drivers/soc/keembay/Kconfig"

For a single 40 line driver, do not make a new directory.  If you end up
with lots in the future, then just move the files.  Don't over-engineer
we have no idea what will actually happen in the future.

>  source "drivers/soc/mediatek/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/renesas/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 36452bed86ef..65c981207283 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -13,6 +13,7 @@ obj-y				+= fsl/
>  obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
>  obj-y				+= imx/
>  obj-$(CONFIG_ARCH_IXP4XX)	+= ixp4xx/
> +obj-y				+= keembay/
>  obj-$(CONFIG_SOC_XWAY)		+= lantiq/
>  obj-y				+= mediatek/
>  obj-y				+= amlogic/
> diff --git a/drivers/soc/keembay/Kconfig b/drivers/soc/keembay/Kconfig
> new file mode 100644
> index 000000000000..2161bce131b3
> --- /dev/null
> +++ b/drivers/soc/keembay/Kconfig
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Keem Bay SoC drivers.
> +#
> +
> +menu "Keem Bay SoC drivers"
> +
> +config KEEMBAY_IMR
> +	bool "Clean-up Keem Bay bootloader IMR at boot"
> +	depends on ARM64
> +	help
> +	  This option makes the Kernel clean up the Isolated Memory Region
> +	  (IMR) set up by Keem Bay bootloader (U-boot) to protect itself during
> +	  early boot.
> +
> +	  The IMR number to be cleaned up is taken from the device tree
> +	  (property 'u-boot-imr' of the 'chosen' node).
> +
> +	  If you are compiling the Kernel for a Keem Bay SoC select Y,
> +	  otherwise select N.

No module support?

What about kernels that support more than one ARM device in the same
kernel, you just broke that :(

> +
> +endmenu
> diff --git a/drivers/soc/keembay/Makefile b/drivers/soc/keembay/Makefile
> new file mode 100644
> index 000000000000..dacfdb9f5fc1
> --- /dev/null
> +++ b/drivers/soc/keembay/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for Keem Bay SoC drivers.
> +#
> +obj-$(CONFIG_KEEMBAY_IMR) += keembay-imr.o
> diff --git a/drivers/soc/keembay/keembay-imr.c b/drivers/soc/keembay/keembay-imr.c
> new file mode 100644
> index 000000000000..eabbdd6e69a7
> --- /dev/null
> +++ b/drivers/soc/keembay/keembay-imr.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019-2020 Intel Corporation
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/printk.h>
> +#include <linux/types.h>
> +
> +/* Keem Bay SiP SVC for clearing an IMR. */
> +#define KMB_SIP_SVC_IMR_CLEAR	0x8200ff13
> +
> +static int __init clear_imr(u64 imr)
> +{
> +	struct arm_smccc_res res = { 0 };
> +
> +	arm_smccc_smc(KMB_SIP_SVC_IMR_CLEAR, imr, 0, 0, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +static int __init kmb_imr_init(void)
> +{
> +	u32 imr;
> +	int rc;
> +
> +	rc = of_property_read_u32(of_chosen, "u-boot-imr", &imr);

Is there a device tree documetnation for this attribute?  You have to
have that approved before you can get your code merged.

> +	if (rc) {
> +		pr_warn("Skipping IMR clean-up: No U-Boot IMR defined in device tree\n");
> +		return 0;
> +	}
> +	pr_info("Disabling Keem Bay U-boot IMR: %u\n", imr);

Drivers are quiet if all is good, don't be noisy for no reason.

> +
> +	return clear_imr(imr);

Did that really need to be a separate function?

> +}
> +early_initcall(kmb_imr_init);

Like I said above, you just broke multi-system kernels by always trying
to do this.  Trigger off of a hardware device that only your platform
has in order to properly load and run.  As-is, you don't want to do
this.

Anyway, Intel owes me more liquor for this review as the resources you
had to get review of this were obviously not taken advantage of.

greg k-h

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Daniele Alessandrelli]

Thanks for your feedback.

> 
> First off, there is a "proper" way to send patches to the kernel
> community that Intel has that I do not think you are
> following.  Please
> work with the Intel "Linux group" to do that first, as odds are they
> would have caught all of these issues beforehand (hint, which is why
> that process is in place...)
> 

I actually followed that process and got the OK to try to upstream.

I think the issues you identified went uncaught mainly due to the
limited Linux ARM expertise within that group (and Intel in general).

Anyway, I'll keep working with our Linux Kernel people to try to reduce
the burden on you and the other kernel maintainers.

> > diff --git a/drivers/soc/keembay/Kconfig
> > b/drivers/soc/keembay/Kconfig
> > new file mode 100644
> > index 000000000000..2161bce131b3
> > --- /dev/null
> > +++ b/drivers/soc/keembay/Kconfig
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# Keem Bay SoC drivers.
> > +#
> > +
> > +menu "Keem Bay SoC drivers"
> > +
> > +config KEEMBAY_IMR
> > +	bool "Clean-up Keem Bay bootloader IMR at boot"
> > +	depends on ARM64
> > +	help
> > +	  This option makes the Kernel clean up the Isolated Memory
> > Region
> > +	  (IMR) set up by Keem Bay bootloader (U-boot) to protect
> > itself during
> > +	  early boot.
> > +
> > +	  The IMR number to be cleaned up is taken from the device tree
> > +	  (property 'u-boot-imr' of the 'chosen' node).
> > +
> > +	  If you are compiling the Kernel for a Keem Bay SoC select Y,
> > +	  otherwise select N.
> 
> No module support?
> 
> What about kernels that support more than one ARM device in the same
> kernel, you just broke that :(
> 

<cut>

> 
> > +}
> > +early_initcall(kmb_imr_init);
> 
> Like I said above, you just broke multi-system kernels by always
> trying
> to do this.  Trigger off of a hardware device that only your platform
> has in order to properly load and run.  As-is, you don't want to do
> this.

My bad, I didn't consider the issue of multi-platform ARM kernels.

The problem is that I need this code to be run early at boot, so I
don't think I can make this a module.

But I'm sure there is a proper way to achieve what I need without
breaking multi-system kernels. I'll do my homework and try to find it.

> 
> Anyway, Intel owes me more liquor for this review as the resources
> you
> had to get review of this were obviously not taken advantage of.
> 
> greg k-h

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Greg Kroah-Hartman]

On Fri, May 01, 2020 at 12:50:36PM +0100, Daniele Alessandrelli wrote:
> Thanks for your feedback.
> 
> > 
> > First off, there is a "proper" way to send patches to the kernel
> > community that Intel has that I do not think you are
> > following.  Please
> > work with the Intel "Linux group" to do that first, as odds are they
> > would have caught all of these issues beforehand (hint, which is why
> > that process is in place...)
> > 
> 
> I actually followed that process and got the OK to try to upstream.

Then someone seriously steered you wrong as I know of one specific thing
you did incorrectly (hint, you needed to get them to sign off on the
patch...)

> I think the issues you identified went uncaught mainly due to the
> limited Linux ARM expertise within that group (and Intel in general).

No, they should know better, if not, there are bigger problems there :(

greg k-h

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Pavel Machek]

On Tue 2020-04-21 17:36:18, Daniele Alessandrelli wrote:
> From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> 
> Keem Bay bootloader sets up a temporary Isolated Memory Region (IMR) to
> protect itself during pre-Linux boot.

What kind of bootloader is the SoC using? Sounds like bootloader responsibility to me...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Pavel Machek]

Hi!

> > Like I said above, you just broke multi-system kernels by always
> > trying
> > to do this.  Trigger off of a hardware device that only your platform
> > has in order to properly load and run.  As-is, you don't want to do
> > this.
> 
> My bad, I didn't consider the issue of multi-platform ARM kernels.
> 
> The problem is that I need this code to be run early at boot, so I
> don't think I can make this a module.

How early is early enough?

What bootloader are you using?

I believe you should simply fix your bootloader not to pass locked memory to the kernel.

Alternatively, take that memory off the "memory available" maps, and only re-add it once
it is usable.

Anything else is dangerous.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Arnd Bergmann]

On Sun, May 24, 2020 at 11:28 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > Like I said above, you just broke multi-system kernels by always
> > > trying
> > > to do this.  Trigger off of a hardware device that only your platform
> > > has in order to properly load and run.  As-is, you don't want to do
> > > this.
> >
> > My bad, I didn't consider the issue of multi-platform ARM kernels.
> >
> > The problem is that I need this code to be run early at boot, so I
> > don't think I can make this a module.
>
> How early is early enough?
>
> What bootloader are you using?
>
> I believe you should simply fix your bootloader not to pass locked memory to the kernel.
>
> Alternatively, take that memory off the "memory available" maps, and only re-add it once
> it is usable.
>
> Anything else is dangerous.

Agreed, this sounds like an incompatible extension of the boot protocol
that we should otherwise not merge.

However, there is also a lot of missing information here, and it is always
possible they are trying to something for a good reason. As long as the
problem that the bootloader is trying to solve is explained well enough
in the changelog, we can discuss it to see how it should be done properly.

       Arnd

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Alessandrelli, Daniele]

Hi Arnd, Pavel,

Thanks for your feedback.

> > > 
> > > The problem is that I need this code to be run early at boot, so
> > > I
> > > don't think I can make this a module.
> > 
> > How early is early enough?

Basically, before any device with direct memory access is activated
(because if anybody, except the ARM CPU, tries to access that memory,
the memory protection mechanism will be triggered and a reboot will
happen).

> > 
> > What bootloader are you using?

U-Boot

> > 
> > I believe you should simply fix your bootloader not to pass locked
> > memory to the kernel.

The bootloader is behaving like that for security reasons, so we'd like
to avoid modifying it. I'll provide more information below.

> > 
> > Alternatively, take that memory off the "memory available" maps,
> > and only re-add it once
> > it is usable.
> > 
> > Anything else is dangerous.

That sounds like an interesting solution, thanks!

Do you know any code that I can use as a reference?

Since, the protected memory is just a few megabytes large, I guess that
in theory we could just have U-Boot mark it as reserved memory and
forget about it; but if there's a way to re-add it back once in Linux
that would be great.

> 
> Agreed, this sounds like an incompatible extension of the boot
> protocol
> that we should otherwise not merge.
> 
> However, there is also a lot of missing information here, and it is
> always
> possible they are trying to something for a good reason. As long as
> the
> problem that the bootloader is trying to solve is explained well
> enough
> in the changelog, we can discuss it to see how it should be done
> properly.


Apologies, I should have provided more information. Here it is :)

Basically, at boot time U-Boot code and core memory (.text, .data,
.bss, etc.) is protected by this Isolated Memory Region (IMR) which
prevents any device or processing units other than the ARM CPU to
access/modify the memory.

This is done for security reasons, to reduce the risks that a potential
attacker can use "hijacked" HW devices to interfere with the boot
process (and break the secure boot flow in place).

Before booting the Kernel, U-Boot sets up a new IMR to protect the
Kernel image (so that the kernel can benefit of a similar protection)
and then starts the kernel, delegating to the kernel the task of
switching off the U-Boot IMR.

U-Boot doesn't turn off its own IMR because doing so would leave a
(tiny) window in which the boot execution flow is not protected.

If you have any additional questions or feedback, just let me know.

Regards,
Daniele








--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Arnd Bergmann]

On Wed, May 27, 2020 at 3:31 PM Alessandrelli, Daniele
<daniele.alessandrelli@intel.com> wrote:
> > > Alternatively, take that memory off the "memory available" maps,
> > > and only re-add it once
> > > it is usable.
> > >
> > > Anything else is dangerous.
>
> That sounds like an interesting solution, thanks!
>
> Do you know any code that I can use as a reference?
>
> Since, the protected memory is just a few megabytes large, I guess that
> in theory we could just have U-Boot mark it as reserved memory and
> forget about it; but if there's a way to re-add it back once in Linux
> that would be great.

Adding it back later on with a loadable device driver should
not be a problem, as this is not a violation of the boot protocol.

> > Agreed, this sounds like an incompatible extension of the boot
> > protocol that we should otherwise not merge.
> >
> > However, there is also a lot of missing information here, and it is
> > always possible they are trying to something for a good reason.
> > As long as the problem that the bootloader is trying to solve is
> > explained well enough in the changelog, we can discuss it to see
> > how it should be done properly.
>
> Apologies, I should have provided more information. Here it is :)
>
> Basically, at boot time U-Boot code and core memory (.text, .data,
> .bss, etc.) is protected by this Isolated Memory Region (IMR) which
> prevents any device or processing units other than the ARM CPU to
> access/modify the memory.
>
> This is done for security reasons, to reduce the risks that a potential
> attacker can use "hijacked" HW devices to interfere with the boot
> process (and break the secure boot flow in place).
>
> Before booting the Kernel, U-Boot sets up a new IMR to protect the
> Kernel image (so that the kernel can benefit of a similar protection)
> and then starts the kernel, delegating to the kernel the task of
> switching off the U-Boot IMR.
>
> U-Boot doesn't turn off its own IMR because doing so would leave a
> (tiny) window in which the boot execution flow is not protected.
>
> If you have any additional questions or feedback, just let me know.

Thank you for the detailed explanation. I've never seen this done
in the Linux boot flow, but I can see how it helps prevent certain
kinds of attacks.

It seems that just reserving the u-boot area and optionally
freeing it later from a driver solves most of your problem.
I have one related question though: if the kernel itself is
protected, does that mean that any driver that does a DMA
to statically allocated memory inside of the kernel is broken
as well? I think this is something that a couple of USB drivers
do, though it is fairly rare. Does u-boot protect both only
the executable sections of the kernel or also data, and does
the hardware block both read and write on the IMR, or just
writes?

        Arnd

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Daniele Alessandrelli]

On Wed, 2020-05-27 at 16:33 +0200, Arnd Bergmann wrote:
> On Wed, May 27, 2020 at 3:31 PM Alessandrelli, Daniele
> <daniele.alessandrelli@intel.com> wrote:
> > > > Alternatively, take that memory off the "memory available"
> > > > maps,
> > > > and only re-add it once
> > > > it is usable.
> > > > 
> > > > Anything else is dangerous.
> > 
> > That sounds like an interesting solution, thanks!
> > 
> > Do you know any code that I can use as a reference?
> > 
> > Since, the protected memory is just a few megabytes large, I guess
> > that
> > in theory we could just have U-Boot mark it as reserved memory and
> > forget about it; but if there's a way to re-add it back once in
> > Linux
> > that would be great.
> 
> Adding it back later on with a loadable device driver should
> not be a problem, as this is not a violation of the boot protocol.

Cool, I'll try to do that then, thanks!

I see two ways to do that though:

1. Create a device driver that gets a reference to the memory region
from its DT node and then re-adds the memory to the pool of available
memory.

2. Use a special "compatible" string for my memory region and create a
driver to handle it.

However, I think that in the second case the driver must be builtin.
Would that be okay?

Also, from a quick look, it seems that I can re-add that memory back by
calling memblock_free() (or a similar memblock_* function). Am I on the
right track?

> 
> > > Agreed, this sounds like an incompatible extension of the boot
> > > protocol that we should otherwise not merge.
> > > 
> > > However, there is also a lot of missing information here, and it
> > > is
> > > always possible they are trying to something for a good reason.
> > > As long as the problem that the bootloader is trying to solve is
> > > explained well enough in the changelog, we can discuss it to see
> > > how it should be done properly.
> > 
> > Apologies, I should have provided more information. Here it is :)
> > 
> > Basically, at boot time U-Boot code and core memory (.text, .data,
> > .bss, etc.) is protected by this Isolated Memory Region (IMR) which
> > prevents any device or processing units other than the ARM CPU to
> > access/modify the memory.
> > 
> > This is done for security reasons, to reduce the risks that a
> > potential
> > attacker can use "hijacked" HW devices to interfere with the boot
> > process (and break the secure boot flow in place).
> > 
> > Before booting the Kernel, U-Boot sets up a new IMR to protect the
> > Kernel image (so that the kernel can benefit of a similar
> > protection)
> > and then starts the kernel, delegating to the kernel the task of
> > switching off the U-Boot IMR.
> > 
> > U-Boot doesn't turn off its own IMR because doing so would leave a
> > (tiny) window in which the boot execution flow is not protected.
> > 
> > If you have any additional questions or feedback, just let me know.
> 
> Thank you for the detailed explanation. I've never seen this done
> in the Linux boot flow, but I can see how it helps prevent certain
> kinds of attacks.
> 
> It seems that just reserving the u-boot area and optionally
> freeing it later from a driver solves most of your problem.
> I have one related question though: if the kernel itself is
> protected, does that mean that any driver that does a DMA
> to statically allocated memory inside of the kernel is broken
> as well? I think this is something that a couple of USB drivers
> do, though it is fairly rare. Does u-boot protect both only
> the executable sections of the kernel or also data, and does
> the hardware block both read and write on the IMR, or just
> writes?

Yes, you're very right. Drivers that do DMA transfers to statically
allocated memory inside the kernel will be broken.

We are currently seeing this with our eMMC driver.

Current solution is to add the eMMC controller to the list of allowed
"agents" for the IMR. This will reduce the level of protection, but
since we expect to have only a few of these exceptions (since, as you
pointed out, drivers that do DMA to static kernel memory seem to be
quite rare), we think that there is still value in having the Kernel
IMR.

Do you see any issue with this?

Regards,
Daniele

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Arnd Bergmann]

On Wed, May 27, 2020 at 7:43 PM Daniele Alessandrelli
<daniele.alessandrelli@linux.intel.com> wrote:
> On Wed, 2020-05-27 at 16:33 +0200, Arnd Bergmann wrote:
> > On Wed, May 27, 2020 at 3:31 PM Alessandrelli, Daniele <daniele.alessandrelli@intel.com> wrote:
> >
> > Adding it back later on with a loadable device driver should
> > not be a problem, as this is not a violation of the boot protocol.
>
> Cool, I'll try to do that then, thanks!
>
> I see two ways to do that though:
>
> 1. Create a device driver that gets a reference to the memory region
> from its DT node and then re-adds the memory to the pool of available
> memory.
>
> 2. Use a special "compatible" string for my memory region and create a
> driver to handle it.

I think the first approach is more common.

> However, I think that in the second case the driver must be builtin.
> Would that be okay?

It's better to avoid that.

> Also, from a quick look, it seems that I can re-add that memory back by
> calling memblock_free() (or a similar memblock_* function). Am I on the
> right track?

I'm not sure if memblock_free() works after early memory initialization
is complete, but I think there is some way to do it later. Maybe try
memblock_free() first, and then look for something else if it doesn't
work.

> > It seems that just reserving the u-boot area and optionally
> > freeing it later from a driver solves most of your problem.
> > I have one related question though: if the kernel itself is
> > protected, does that mean that any driver that does a DMA
> > to statically allocated memory inside of the kernel is broken
> > as well? I think this is something that a couple of USB drivers
> > do, though it is fairly rare. Does u-boot protect both only
> > the executable sections of the kernel or also data, and does
> > the hardware block both read and write on the IMR, or just
> > writes?
>
> Yes, you're very right. Drivers that do DMA transfers to statically
> allocated memory inside the kernel will be broken.
>
> We are currently seeing this with our eMMC driver.
>
> Current solution is to add the eMMC controller to the list of allowed
> "agents" for the IMR. This will reduce the level of protection, but
> since we expect to have only a few of these exceptions (since, as you
> pointed out, drivers that do DMA to static kernel memory seem to be
> quite rare), we think that there is still value in having the Kernel
> IMR.
>
> Do you see any issue with this?

I think you should try to fix the driver rather than making an
exception for it. Hot-pluggable drivers are a much more interesting
case I think, because on the one hand you have no idea what
users might want to plug in legitimately, but on the other hand
those are also the most likely attack vectors (driver bugs for
random USB drivers overwriting kernel memory when faced with
malicious hardware) that this feature is trying to prevent.

I also wonder whether we should do something in the normal
iommu code that prevents one from mapping a page that the
kernel would consider as protected (kernel .text, freed memory,
...) into the iommu in the first place.

        Arnd

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

Hi!

> > Agreed, this sounds like an incompatible extension of the boot
> > protocol
> > that we should otherwise not merge.
> > 
> > However, there is also a lot of missing information here, and it is
> > always
> > possible they are trying to something for a good reason. As long as
> > the
> > problem that the bootloader is trying to solve is explained well
> > enough
> > in the changelog, we can discuss it to see how it should be done
> > properly.
> 
> 
> Apologies, I should have provided more information. Here it is :)
> 
> Basically, at boot time U-Boot code and core memory (.text, .data,
> .bss, etc.) is protected by this Isolated Memory Region (IMR) which
> prevents any device or processing units other than the ARM CPU to
> access/modify the memory.
> 
> This is done for security reasons, to reduce the risks that a potential
> attacker can use "hijacked" HW devices to interfere with the boot
> process (and break the secure boot flow in place).

Dunno. You disable that after boot anyway. Whether it is disabled just
before starting kernel or just after it makes very little difference.

Plus, I'm not sure if this has much security value at all. If I can
corrupt data u-boot works _with_ (such as kernel, dtb), I'll take over
the system anyway.

IOW I believe the best/simplest way is to simply disable this in
u-boot before jumping to kernel entrypoint.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Daniele Alessandrelli]

On Wed, 2020-05-27 at 20:59 +0200, Arnd Bergmann wrote:
> On Wed, May 27, 2020 at 7:43 PM Daniele Alessandrelli
> <daniele.alessandrelli@linux.intel.com> wrote:
> > On Wed, 2020-05-27 at 16:33 +0200, Arnd Bergmann wrote:
> > > On Wed, May 27, 2020 at 3:31 PM Alessandrelli, Daniele <
> > > daniele.alessandrelli@intel.com> wrote:
> > > 
> > > Adding it back later on with a loadable device driver should
> > > not be a problem, as this is not a violation of the boot
> > > protocol.
> > 
> > Cool, I'll try to do that then, thanks!
> > 
> > I see two ways to do that though:
> > 
> > 1. Create a device driver that gets a reference to the memory
> > region
> > from its DT node and then re-adds the memory to the pool of
> > available
> > memory.
> > 
> > 2. Use a special "compatible" string for my memory region and
> > create a
> > driver to handle it.
> 
> I think the first approach is more common.
> 
> > However, I think that in the second case the driver must be
> > builtin.
> > Would that be okay?
> 
> It's better to avoid that.
> 
> > Also, from a quick look, it seems that I can re-add that memory
> > back by
> > calling memblock_free() (or a similar memblock_* function). Am I on
> > the
> > right track?
> 
> I'm not sure if memblock_free() works after early memory
> initialization
> is complete, but I think there is some way to do it later. Maybe try
> memblock_free() first, and then look for something else if it doesn't
> work.
> 

Brilliant. I will create a new patch using the 1st approach and see if
memblock_free() works; if not, I will look for something else.

Thanks a lot for your valuable feedback and advice.

> > > It seems that just reserving the u-boot area and optionally
> > > freeing it later from a driver solves most of your problem.
> > > I have one related question though: if the kernel itself is
> > > protected, does that mean that any driver that does a DMA
> > > to statically allocated memory inside of the kernel is broken
> > > as well? I think this is something that a couple of USB drivers
> > > do, though it is fairly rare. Does u-boot protect both only
> > > the executable sections of the kernel or also data, and does
> > > the hardware block both read and write on the IMR, or just
> > > writes?
> > 
> > Yes, you're very right. Drivers that do DMA transfers to statically
> > allocated memory inside the kernel will be broken.
> > 
> > We are currently seeing this with our eMMC driver.
> > 
> > Current solution is to add the eMMC controller to the list of
> > allowed
> > "agents" for the IMR. This will reduce the level of protection, but
> > since we expect to have only a few of these exceptions (since, as
> > you
> > pointed out, drivers that do DMA to static kernel memory seem to be
> > quite rare), we think that there is still value in having the
> > Kernel
> > IMR.
> > 
> > Do you see any issue with this?
> 
> I think you should try to fix the driver rather than making an
> exception for it.

Yes, we'll look into that.

> Hot-pluggable drivers are a much more interesting
> case I think, because on the one hand you have no idea what
> users might want to plug in legitimately, but on the other hand
> those are also the most likely attack vectors (driver bugs for
> random USB drivers overwriting kernel memory when faced with
> malicious hardware) that this feature is trying to prevent.
> 
> I also wonder whether we should do something in the normal
> iommu code that prevents one from mapping a page that the
> kernel would consider as protected (kernel .text, freed memory,
> ...) into the iommu in the first place.

Sounds like an iteresting security feature; expecially because it would
apply to any hardware.

> 
>         Arnd

Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Daniele Alessandrelli]

On Thu, 2020-05-28 at 13:22 +0200, Pavel Machek wrote:
> Hi!
> 
> > > Agreed, this sounds like an incompatible extension of the boot
> > > protocol
> > > that we should otherwise not merge.
> > > 
> > > However, there is also a lot of missing information here, and it
> > > is
> > > always
> > > possible they are trying to something for a good reason. As long
> > > as
> > > the
> > > problem that the bootloader is trying to solve is explained well
> > > enough
> > > in the changelog, we can discuss it to see how it should be done
> > > properly.
> > 
> > Apologies, I should have provided more information. Here it is :)
> > 
> > Basically, at boot time U-Boot code and core memory (.text, .data,
> > .bss, etc.) is protected by this Isolated Memory Region (IMR) which
> > prevents any device or processing units other than the ARM CPU to
> > access/modify the memory.
> > 
> > This is done for security reasons, to reduce the risks that a
> > potential
> > attacker can use "hijacked" HW devices to interfere with the boot
> > process (and break the secure boot flow in place).
> 
> Dunno. You disable that after boot anyway. Whether it is disabled
> just before starting kernel or just after it makes very little
> difference.

Not sure I get your point. Disabling it while U-Boot is still running
poses a security risk (even if arguably tiny), while doing it once the
the Kernel is running is  totally safe. So, I'd prefer to do it in the
Kernel, unless practical reasons prevent it.

> 
> Plus, I'm not sure if this has much security value at all. If I can
> corrupt data u-boot works _with_ (such as kernel, dtb), I'll take
> over the system anyway.

True, U-Boot data needs to be protected too and, in fact, we're trying
to do that as well. Other IMRs are used to protect the kernel, dtb, and
other critical memory sections.

> 
> IOW I believe the best/simplest way is to simply disable this in
> u-boot before jumping to kernel entrypoint.

Yes, that's definitely the simplest solution, but, IMO, not the safest
one. So, I'd prefer to build on your initial suggestion and Arnd's
advice and create a new device driver to disable the IMR once Linux is
running. But, yes, if that eventually proves unfeasible, I might just
have the bootloader disable the protection right before booting the OS.

> 
> Best regards,
> 									
> Pavel
>