[PATCH v2 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn

[PATCH v2 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn

[Stefan Kerkmann]

This series corrects the HABv4 rom vector table and syncs it with the
offical NXP documentation and switches the imx8mm and imx8mn socs from
the `imx8m_read_sram_events` ram parsing implementation to the one found
in the HABv4 rom. Calling the boot rom previously caused hangs which was
the root cause for the ram parsing workaround, with the corrections to
the API these hangs were no longer observed.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
Changes in v2:
- Fixed coding style from review comments
- Added FSL_SIP_HAB_AUTH_IMG_NO_DCD to hab_sip_cmd found in TF-A 2.10
- Added imx8mp to known good targets
- Link to v1: https://lore.kernel.org/r/20240111-fix-habv4-event-report-v1-0-15d9a990f01e@pengutronix.de

---
Stefan Kerkmann (2):
      habv4: correct habv4 rom vector table
      habv4: use hab rom implementation of report_event

 drivers/hab/habv4.c | 102 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 72 insertions(+), 30 deletions(-)
---
base-commit: 3840c1cd99768c8755c073f3749829878670b3c5
change-id: 20240111-fix-habv4-event-report-aa1cc1fb7c95

Best regards,
-- 
Stefan Kerkmann <s.kerkmann@pengutronix.de>

[PATCH v2 1/2] habv4: correct habv4 rom vector table

[Stefan Kerkmann]

All function signatures have been taken from the NXP manual "High
Assurance Boot Version 4 Application Programming Interface Reference
Manual" revision 1.4 under section "4.5 ROM vector table". A copy can be
obtained from the imx code signing tool (imx-cst).

The HAB SIP enum was extended with FSL_SIP_HAB_AUTH_IMG_NO_DCD which is
supported by the upstream TF-A release 2.10.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 drivers/hab/habv4.c | 54 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index ed6d4db77c..92bee8399d 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -144,31 +144,45 @@ struct hab_header {
 	uint8_t par;
 } __packed;
 
-typedef enum hab_status hab_loader_callback_fn(void **start, uint32_t *bytes, const void *boot_data);
+typedef enum hab_status hab_loader_callback_fn(void **start, size_t *bytes, const void *boot_data);
+typedef void hab_image_entry_fn(void);
 
+/* This table is constructed from the NXP manual "High Assurance Boot Version 4
+ * Application Programming Interface Reference Manual", section 4.5 ROM vector
+ * table. Revision 1.4 */
 struct habv4_rvt {
 	struct hab_header header;
 	enum hab_status (*entry)(void);
 	enum hab_status (*exit)(void);
-	enum hab_status (*check_target)(enum hab_target target, const void *start, uint32_t bytes);
-	void *(*authenticate_image)(uint8_t cid, uint32_t ivt_offset, void **start, uint32_t *bytes, hab_loader_callback_fn *loader);
-	enum hab_status (*run_dcd)(const void *dcd);
-	enum hab_status (*run_csf)(const void *csf, uint8_t cid);
+	enum hab_status (*check_target)(enum hab_target target, const void *start, size_t bytes);
+	void *(*authenticate_image)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn *loader);
+	enum hab_status (*run_dcd)(const uint8_t *dcd);
+	enum hab_status (*run_csf)(const uint8_t *csf, uint8_t cid, uint32_t srkmask);
 	enum hab_status (*assert)(enum hab_assertion assertion, const void *data, uint32_t count);
-	enum hab_status (*report_event)(enum hab_status status, uint32_t index, void *event, uint32_t *bytes);
+	enum hab_status (*report_event)(enum hab_status status, uint32_t index, uint8_t *event, size_t *bytes);
 	enum hab_status (*report_status)(enum hab_config *config, enum habv4_state *state);
 	void (*failsafe)(void);
+	hab_image_entry_fn* (* authenticate_image_no_dcd)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn *loader);
+	uint32_t (*get_version)(void);
+	enum hab_status (*authenticate_container)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn *loader, uint32_t srkmask, int skip_dcd);
 } __packed;
 
-#define FSL_SIP_HAB             0xC2000007
-#define FSL_SIP_HAB_AUTHENTICATE        0x00
-#define FSL_SIP_HAB_ENTRY               0x01
-#define FSL_SIP_HAB_EXIT                0x02
-#define FSL_SIP_HAB_REPORT_EVENT        0x03
-#define FSL_SIP_HAB_REPORT_STATUS       0x04
-#define FSL_SIP_HAB_FAILSAFE            0x05
-#define FSL_SIP_HAB_CHECK_TARGET        0x06
-#define FSL_SIP_HAB_GET_VERSION		0x07
+#define FSL_SIP_HAB 0xC2000007
+
+/* These values correspondent to the jump table found in the upstream TF-A
+ * version 2.10 `imx_hab_handler`, not all HAB rom functions are supported yet.
+ * */
+enum hab_sip_cmd {
+	FSL_SIP_HAB_AUTHENTICATE = 0x00,
+	FSL_SIP_HAB_ENTRY = 0x01,
+	FSL_SIP_HAB_EXIT = 0x02,
+	FSL_SIP_HAB_REPORT_EVENT = 0x03,
+	FSL_SIP_HAB_REPORT_STATUS = 0x04,
+	FSL_SIP_HAB_FAILSAFE = 0x05,
+	FSL_SIP_HAB_CHECK_TARGET = 0x06,
+	FSL_SIP_HAB_GET_VERSION = 0x07,
+	FSL_SIP_HAB_AUTH_IMG_NO_DCD = 0x08,
+};
 
 static enum hab_status hab_sip_report_status(enum hab_config *config,
 					     enum habv4_state *state)
@@ -211,8 +225,8 @@ static uint32_t hab_sip_get_version(void)
 #define IMX8MP_ROM_OCRAM_ADDRESS	0x90D040
 
 static enum hab_status imx8m_read_sram_events(enum hab_status status,
-					     uint32_t index, void *event,
-					     uint32_t *bytes)
+					     uint32_t index, uint8_t *event,
+					     size_t *bytes)
 {
 	struct hab_event_record *events[10];
 	int num_events = 0;
@@ -478,7 +492,7 @@ static void habv4_display_event_record(struct hab_event_record *record)
 	pr_err("Engine: %s (0x%02x)\n", habv4_get_engine_str(record->engine), record->engine);
 }
 
-static void habv4_display_event(uint8_t *data, uint32_t len)
+static void habv4_display_event(uint8_t *data, size_t len)
 {
 	unsigned int i;
 
@@ -525,7 +539,7 @@ static bool is_known_rng_fail_event(const uint8_t *data, size_t len)
 	return false;
 }
 
-static uint8_t *hab_get_event(const struct habv4_rvt *rvt, int index, int *len)
+static uint8_t *hab_get_event(const struct habv4_rvt *rvt, int index, size_t *len)
 {
 	enum hab_status err;
 	uint8_t *buf;
@@ -558,7 +572,7 @@ int habv4_get_state(void)
 static int habv4_get_status(const struct habv4_rvt *rvt)
 {
 	uint8_t *data;
-	uint32_t len;
+	size_t len;
 	int i;
 	enum hab_status status;
 	enum hab_config config = 0x0;

-- 
2.39.2

[PATCH v2 2/2] habv4: use hab rom implementation of report_event

[Stefan Kerkmann]

The existing habv4 rom vector table had some mismatches in the API of
the function pointers which broke calling into the HAB rom - mainly
observed with the `report_event` function. The suspected culprit here is
the `bytes` pointer which was `uint32_t*` vs. the documented `size_t*`.

When compiled using the ILP32 data model e.g. for 32-Bit systems both
referrenced values have the same width, but once compiled for (I)LP64
they differ as `size_t` is 64-Bit wide there. This seems to trigger a
memory corruption once that pointer is passed to the HAB boot rom code
and dereferenced there, the root cause wasn't investigated further
though.

As this implementation has only been tested on imx8mm, imx8nm and imx8mp
boards I'm beeing defensive and only enable it for these targets. Once
all SOCs of the family have been verified to work correctly the OCRAM
readout workaround can be removed.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 drivers/hab/habv4.c | 48 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index 92bee8399d..4e401ca9d3 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -184,6 +184,33 @@ enum hab_sip_cmd {
 	FSL_SIP_HAB_AUTH_IMG_NO_DCD = 0x08,
 };
 
+static enum hab_status hab_sip_report_event(enum hab_status status,
+					    uint32_t index, uint8_t *event,
+					    size_t *bytes)
+{
+	struct arm_smccc_res res;
+
+	v8_flush_dcache_range((unsigned long)bytes,
+			      (unsigned long)bytes + sizeof(*bytes));
+
+	if (event)
+		v8_flush_dcache_range((unsigned long)event,
+				      (unsigned long)event + *bytes);
+
+	arm_smccc_smc(FSL_SIP_HAB, FSL_SIP_HAB_REPORT_EVENT,
+		      (unsigned long)index, (unsigned long)event,
+		      (unsigned long)bytes, 0, 0, 0, &res);
+
+	v8_inv_dcache_range((unsigned long)bytes,
+			    (unsigned long)bytes + sizeof(*bytes));
+
+	if (event)
+		v8_inv_dcache_range((unsigned long)event,
+				    (unsigned long)event + *bytes);
+
+	return (enum hab_status)res.a0;
+}
+
 static enum hab_status hab_sip_report_status(enum hab_config *config,
 					     enum habv4_state *state)
 {
@@ -220,9 +247,6 @@ static uint32_t hab_sip_get_version(void)
 #define HABV4_EVENT_MAX_LEN		0x80
 
 #define IMX8MQ_ROM_OCRAM_ADDRESS	0x9061C0
-#define IMX8MM_ROM_OCRAM_ADDRESS	0x908040
-#define IMX8MN_ROM_OCRAM_ADDRESS	0x908040
-#define IMX8MP_ROM_OCRAM_ADDRESS	0x90D040
 
 static enum hab_status imx8m_read_sram_events(enum hab_status status,
 					     uint32_t index, uint8_t *event,
@@ -239,12 +263,6 @@ static enum hab_status imx8m_read_sram_events(enum hab_status status,
 
 	if (cpu_is_mx8mq())
 		sram = (char *)IMX8MQ_ROM_OCRAM_ADDRESS;
-	else if (cpu_is_mx8mm())
-		sram = (char *)IMX8MM_ROM_OCRAM_ADDRESS;
-	else if (cpu_is_mx8mn())
-		sram = (char *)IMX8MN_ROM_OCRAM_ADDRESS;
-	else if (cpu_is_mx8mp())
-		sram = (char *)IMX8MP_ROM_OCRAM_ADDRESS;
 	else
 		return HAB_STATUS_FAILURE;
 
@@ -296,9 +314,19 @@ static enum hab_status imx8m_read_sram_events(enum hab_status status,
 	return HAB_STATUS_FAILURE;
 }
 
+static enum hab_status imx8m_report_event(enum hab_status status,
+					  uint32_t index, uint8_t *event,
+					  size_t *bytes)
+{
+	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_imx8mp())
+		return hab_sip_report_event(status, index, event, bytes);
+	else
+		return imx8m_read_sram_events(status, index, event, bytes);
+}
+
 struct habv4_rvt hab_smc_ops = {
 	.header = { .tag = 0xdd },
-	.report_event = imx8m_read_sram_events,
+	.report_event = imx8m_report_event,
 	.report_status = hab_sip_report_status,
 };
 

-- 
2.39.2

Re: [PATCH v2 2/2] habv4: use hab rom implementation of report_event

[Stefan Kerkmann]

On 12.01.24 16:21, Stefan Kerkmann wrote:
> The existing habv4 rom vector table had some mismatches in the API of
> the function pointers which broke calling into the HAB rom - mainly
> observed with the `report_event` function. The suspected culprit here is
> the `bytes` pointer which was `uint32_t*` vs. the documented `size_t*`.
> 
> When compiled using the ILP32 data model e.g. for 32-Bit systems both
> referrenced values have the same width, but once compiled for (I)LP64
> they differ as `size_t` is 64-Bit wide there. This seems to trigger a
> memory corruption once that pointer is passed to the HAB boot rom code
> and dereferenced there, the root cause wasn't investigated further
> though.
> 
> As this implementation has only been tested on imx8mm, imx8nm and imx8mp
> boards I'm beeing defensive and only enable it for these targets. Once
> all SOCs of the family have been verified to work correctly the OCRAM
> readout workaround can be removed.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>   drivers/hab/habv4.c | 48 ++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index 92bee8399d..4e401ca9d3 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -184,6 +184,33 @@ enum hab_sip_cmd {
>   	FSL_SIP_HAB_AUTH_IMG_NO_DCD = 0x08,
>   };
>   
> +static enum hab_status hab_sip_report_event(enum hab_status status,
> +					    uint32_t index, uint8_t *event,
> +					    size_t *bytes)
> +{
> +	struct arm_smccc_res res;
> +
> +	v8_flush_dcache_range((unsigned long)bytes,
> +			      (unsigned long)bytes + sizeof(*bytes));
> +
> +	if (event)
> +		v8_flush_dcache_range((unsigned long)event,
> +				      (unsigned long)event + *bytes);
> +
> +	arm_smccc_smc(FSL_SIP_HAB, FSL_SIP_HAB_REPORT_EVENT,
> +		      (unsigned long)index, (unsigned long)event,
> +		      (unsigned long)bytes, 0, 0, 0, &res);
> +
> +	v8_inv_dcache_range((unsigned long)bytes,
> +			    (unsigned long)bytes + sizeof(*bytes));
> +
> +	if (event)
> +		v8_inv_dcache_range((unsigned long)event,
> +				    (unsigned long)event + *bytes);
> +
> +	return (enum hab_status)res.a0;
> +}
> +
>   static enum hab_status hab_sip_report_status(enum hab_config *config,
>   					     enum habv4_state *state)
>   {
> @@ -220,9 +247,6 @@ static uint32_t hab_sip_get_version(void)
>   #define HABV4_EVENT_MAX_LEN		0x80
>   
>   #define IMX8MQ_ROM_OCRAM_ADDRESS	0x9061C0
> -#define IMX8MM_ROM_OCRAM_ADDRESS	0x908040
> -#define IMX8MN_ROM_OCRAM_ADDRESS	0x908040
> -#define IMX8MP_ROM_OCRAM_ADDRESS	0x90D040
>   
>   static enum hab_status imx8m_read_sram_events(enum hab_status status,
>   					     uint32_t index, uint8_t *event,
> @@ -239,12 +263,6 @@ static enum hab_status imx8m_read_sram_events(enum hab_status status,
>   
>   	if (cpu_is_mx8mq())
>   		sram = (char *)IMX8MQ_ROM_OCRAM_ADDRESS;
> -	else if (cpu_is_mx8mm())
> -		sram = (char *)IMX8MM_ROM_OCRAM_ADDRESS;
> -	else if (cpu_is_mx8mn())
> -		sram = (char *)IMX8MN_ROM_OCRAM_ADDRESS;
> -	else if (cpu_is_mx8mp())
> -		sram = (char *)IMX8MP_ROM_OCRAM_ADDRESS;
>   	else
>   		return HAB_STATUS_FAILURE;
>   
> @@ -296,9 +314,19 @@ static enum hab_status imx8m_read_sram_events(enum hab_status status,
>   	return HAB_STATUS_FAILURE;
>   }
>   
> +static enum hab_status imx8m_report_event(enum hab_status status,
> +					  uint32_t index, uint8_t *event,
> +					  size_t *bytes)
> +{
> +	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_imx8mp())

There is a typo in the condition, that I somehow introduced wrangling 
testing branches. It should be `cpu_is_mx8mp()` not `cpu_is_imx8mp()` 
I'll send another series with fix with the upcoming review feedback 
(which is to come as I was told).

> +		return hab_sip_report_event(status, index, event, bytes);
> +	else
> +		return imx8m_read_sram_events(status, index, event, bytes);
> +}
> +
>   struct habv4_rvt hab_smc_ops = {
>   	.header = { .tag = 0xdd },
> -	.report_event = imx8m_read_sram_events,
> +	.report_event = imx8m_report_event,
>   	.report_status = hab_sip_report_status,
>   };
>   
> 

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |

Re: [PATCH v2 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn

[Sascha Hauer]

On Fri, 12 Jan 2024 16:21:06 +0100, Stefan Kerkmann wrote:
> This series corrects the HABv4 rom vector table and syncs it with the
> offical NXP documentation and switches the imx8mm and imx8mn socs from
> the `imx8m_read_sram_events` ram parsing implementation to the one found
> in the HABv4 rom. Calling the boot rom previously caused hangs which was
> the root cause for the ram parsing workaround, with the corrections to
> the API these hangs were no longer observed.
> 
> [...]

Applied, thanks!

[1/2] habv4: correct habv4 rom vector table
      https://git.pengutronix.de/cgit/barebox/commit/?id=8f8178a8a82a (link may not be stable)
[2/2] habv4: use hab rom implementation of report_event
      https://git.pengutronix.de/cgit/barebox/commit/?id=1853e97dc325 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH v2 2/2] habv4: use hab rom implementation of report_event

[Sascha Hauer]

On Fri, Jan 12, 2024 at 05:29:58PM +0100, Stefan Kerkmann wrote:
> On 12.01.24 16:21, Stefan Kerkmann wrote:
> > The existing habv4 rom vector table had some mismatches in the API of
> > the function pointers which broke calling into the HAB rom - mainly
> > observed with the `report_event` function. The suspected culprit here is
> > the `bytes` pointer which was `uint32_t*` vs. the documented `size_t*`.
> > 
> > When compiled using the ILP32 data model e.g. for 32-Bit systems both
> > referrenced values have the same width, but once compiled for (I)LP64
> > they differ as `size_t` is 64-Bit wide there. This seems to trigger a
> > memory corruption once that pointer is passed to the HAB boot rom code
> > and dereferenced there, the root cause wasn't investigated further
> > though.
> > 
> > As this implementation has only been tested on imx8mm, imx8nm and imx8mp
> > boards I'm beeing defensive and only enable it for these targets. Once
> > all SOCs of the family have been verified to work correctly the OCRAM
> > readout workaround can be removed.
> > 
> > Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> > ---
> >   drivers/hab/habv4.c | 48 ++++++++++++++++++++++++++++++++++++++----------
> >   1 file changed, 38 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> > index 92bee8399d..4e401ca9d3 100644
> > --- a/drivers/hab/habv4.c
> > +++ b/drivers/hab/habv4.c
> > @@ -184,6 +184,33 @@ enum hab_sip_cmd {
> >   	FSL_SIP_HAB_AUTH_IMG_NO_DCD = 0x08,
> >   };
> > +static enum hab_status hab_sip_report_event(enum hab_status status,
> > +					    uint32_t index, uint8_t *event,
> > +					    size_t *bytes)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	v8_flush_dcache_range((unsigned long)bytes,
> > +			      (unsigned long)bytes + sizeof(*bytes));
> > +
> > +	if (event)
> > +		v8_flush_dcache_range((unsigned long)event,
> > +				      (unsigned long)event + *bytes);
> > +
> > +	arm_smccc_smc(FSL_SIP_HAB, FSL_SIP_HAB_REPORT_EVENT,
> > +		      (unsigned long)index, (unsigned long)event,
> > +		      (unsigned long)bytes, 0, 0, 0, &res);
> > +
> > +	v8_inv_dcache_range((unsigned long)bytes,
> > +			    (unsigned long)bytes + sizeof(*bytes));
> > +
> > +	if (event)
> > +		v8_inv_dcache_range((unsigned long)event,
> > +				    (unsigned long)event + *bytes);
> > +
> > +	return (enum hab_status)res.a0;
> > +}
> > +
> >   static enum hab_status hab_sip_report_status(enum hab_config *config,
> >   					     enum habv4_state *state)
> >   {
> > @@ -220,9 +247,6 @@ static uint32_t hab_sip_get_version(void)
> >   #define HABV4_EVENT_MAX_LEN		0x80
> >   #define IMX8MQ_ROM_OCRAM_ADDRESS	0x9061C0
> > -#define IMX8MM_ROM_OCRAM_ADDRESS	0x908040
> > -#define IMX8MN_ROM_OCRAM_ADDRESS	0x908040
> > -#define IMX8MP_ROM_OCRAM_ADDRESS	0x90D040
> >   static enum hab_status imx8m_read_sram_events(enum hab_status status,
> >   					     uint32_t index, uint8_t *event,
> > @@ -239,12 +263,6 @@ static enum hab_status imx8m_read_sram_events(enum hab_status status,
> >   	if (cpu_is_mx8mq())
> >   		sram = (char *)IMX8MQ_ROM_OCRAM_ADDRESS;
> > -	else if (cpu_is_mx8mm())
> > -		sram = (char *)IMX8MM_ROM_OCRAM_ADDRESS;
> > -	else if (cpu_is_mx8mn())
> > -		sram = (char *)IMX8MN_ROM_OCRAM_ADDRESS;
> > -	else if (cpu_is_mx8mp())
> > -		sram = (char *)IMX8MP_ROM_OCRAM_ADDRESS;
> >   	else
> >   		return HAB_STATUS_FAILURE;
> > @@ -296,9 +314,19 @@ static enum hab_status imx8m_read_sram_events(enum hab_status status,
> >   	return HAB_STATUS_FAILURE;
> >   }
> > +static enum hab_status imx8m_report_event(enum hab_status status,
> > +					  uint32_t index, uint8_t *event,
> > +					  size_t *bytes)
> > +{
> > +	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_imx8mp())
> 
> There is a typo in the condition, that I somehow introduced wrangling
> testing branches. It should be `cpu_is_mx8mp()` not `cpu_is_imx8mp()`

Fixed while applying.

> I'll
> send another series with fix with the upcoming review feedback (which is to
> come as I was told).

I haven't read this part carefully enough. Ok, let's see what happens, I
can replace this series with another one should there be more review
feedback.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn

[Sascha Hauer]

On Fri, 12 Jan 2024 16:21:06 +0100, Stefan Kerkmann wrote:
> This series corrects the HABv4 rom vector table and syncs it with the
> offical NXP documentation and switches the imx8mm and imx8mn socs from
> the `imx8m_read_sram_events` ram parsing implementation to the one found
> in the HABv4 rom. Calling the boot rom previously caused hangs which was
> the root cause for the ram parsing workaround, with the corrections to
> the API these hangs were no longer observed.
> 
> [...]

Applied, thanks!

[1/2] habv4: correct habv4 rom vector table
      https://git.pengutronix.de/cgit/barebox/commit/?id=b2a4fe2c3494 (link may not be stable)
[2/2] habv4: use hab rom implementation of report_event
      https://git.pengutronix.de/cgit/barebox/commit/?id=02a795cc0866 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH v2 2/2] habv4: use hab rom implementation of report_event

[Ahmad Fatoum]

Hello Stefan,

On 12.01.24 16:21, Stefan Kerkmann wrote:
> +static enum hab_status hab_sip_report_event(enum hab_status status,
> +					    uint32_t index, uint8_t *event,
> +					    size_t *bytes)
> +{
> +	struct arm_smccc_res res;
> +
> +	v8_flush_dcache_range((unsigned long)bytes,
> +			      (unsigned long)bytes + sizeof(*bytes));
> +
> +	if (event)
> +		v8_flush_dcache_range((unsigned long)event,
> +				      (unsigned long)event + *bytes);

I am not too happy about the cache maintenance here. *event and *bytes
are both stack memory, which share a cache line with other stack variables.

This issue exists in hab_sip_report_status too, so this need not delay
application of the series, but it would nice to get this cleaned up,
eventually.

A first attempt was here:
https://lore.barebox.org/barebox/20230921095649.310666-1-a.fatoum@pengutronix.de/

I am also unsure if cache maintenance is correct, see:
https://lists.trustedfirmware.org/archives/list/tf-a@lists.trustedfirmware.org/message/D3PIAW7G2B3JQIH5BGMUZZKHPGNMXUUT/

> +static enum hab_status imx8m_report_event(enum hab_status status,
> +					  uint32_t index, uint8_t *event,
> +					  size_t *bytes)
> +{
> +	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_imx8mp())

I suggest we swap the if else clauses and use cpu_is_mx8mq(). imx8m_read_sram_events
only supports that one SoC now and it's likely that new SoCs (e.g. i.MX9) will
also reuse hab_sip_report_event. Could you send a fixup?

Thanks,
Ahmad

> +		return hab_sip_report_event(status, index, event, bytes);
> +	else
> +		return imx8m_read_sram_events(status, index, event, bytes);
> +}
> +
>  struct habv4_rvt hab_smc_ops = {
>  	.header = { .tag = 0xdd },
> -	.report_event = imx8m_read_sram_events,
> +	.report_event = imx8m_report_event,
>  	.report_status = hab_sip_report_status,
>  };
>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/2] habv4: use hab rom implementation of report_event

[Sascha Hauer]

On Tue, Jan 16, 2024 at 09:13:48AM +0100, Ahmad Fatoum wrote:
> Hello Stefan,
> 
> On 12.01.24 16:21, Stefan Kerkmann wrote:
> > +static enum hab_status hab_sip_report_event(enum hab_status status,
> > +					    uint32_t index, uint8_t *event,
> > +					    size_t *bytes)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	v8_flush_dcache_range((unsigned long)bytes,
> > +			      (unsigned long)bytes + sizeof(*bytes));
> > +
> > +	if (event)
> > +		v8_flush_dcache_range((unsigned long)event,
> > +				      (unsigned long)event + *bytes);
> 
> I am not too happy about the cache maintenance here. *event and *bytes
> are both stack memory, which share a cache line with other stack variables.
> 
> This issue exists in hab_sip_report_status too, so this need not delay
> application of the series, but it would nice to get this cleaned up,
> eventually.
> 
> A first attempt was here:
> https://lore.barebox.org/barebox/20230921095649.310666-1-a.fatoum@pengutronix.de/
> 
> I am also unsure if cache maintenance is correct, see:
> https://lists.trustedfirmware.org/archives/list/tf-a@lists.trustedfirmware.org/message/D3PIAW7G2B3JQIH5BGMUZZKHPGNMXUUT/
> 
> > +static enum hab_status imx8m_report_event(enum hab_status status,
> > +					  uint32_t index, uint8_t *event,
> > +					  size_t *bytes)
> > +{
> > +	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_imx8mp())
> 
> I suggest we swap the if else clauses and use cpu_is_mx8mq(). imx8m_read_sram_events
> only supports that one SoC now and it's likely that new SoCs (e.g. i.MX9) will
> also reuse hab_sip_report_event. Could you send a fixup?

I fixed this part up here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/2] habv4: use hab rom implementation of report_event

[Stefan Kerkmann]

Hello Ahmad, Hello Sascha,

On 17.01.24 08:20, Sascha Hauer wrote:
> On Tue, Jan 16, 2024 at 09:13:48AM +0100, Ahmad Fatoum wrote:
>> Hello Stefan,
>>
>> On 12.01.24 16:21, Stefan Kerkmann wrote:
>>> +static enum hab_status hab_sip_report_event(enum hab_status status,
>>> +					    uint32_t index, uint8_t *event,
>>> +					    size_t *bytes)
>>> +{
>>> +	struct arm_smccc_res res;
>>> +
>>> +	v8_flush_dcache_range((unsigned long)bytes,
>>> +			      (unsigned long)bytes + sizeof(*bytes));
>>> +
>>> +	if (event)
>>> +		v8_flush_dcache_range((unsigned long)event,
>>> +				      (unsigned long)event + *bytes);
>>
>> I am not too happy about the cache maintenance here. *event and *bytes
>> are both stack memory, which share a cache line with other stack variables.
>>
>> This issue exists in hab_sip_report_status too, so this need not delay
>> application of the series, but it would nice to get this cleaned up,
>> eventually.
>>
>> A first attempt was here:
>> https://lore.barebox.org/barebox/20230921095649.310666-1-a.fatoum@pengutronix.de/
>>
>> I am also unsure if cache maintenance is correct, see:
>> https://lists.trustedfirmware.org/archives/list/tf-a@lists.trustedfirmware.org/message/D3PIAW7G2B3JQIH5BGMUZZKHPGNMXUUT/
>>

When implementing the cache maintenance I had a quick talk with a 
colleague and it wasn't clear if this is needed at all. In the end I 
opted for "better safe than sorry". I suggest to wait a bit if there is 
a definitive answer from the TF-A list and remove it altogether if it 
turns out to be unnecessary.

>>> +static enum hab_status imx8m_report_event(enum hab_status status,
>>> +					  uint32_t index, uint8_t *event,
>>> +					  size_t *bytes)
>>> +{
>>> +	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_imx8mp())
>>
>> I suggest we swap the if else clauses and use cpu_is_mx8mq(). imx8m_read_sram_events
>> only supports that one SoC now and it's likely that new SoCs (e.g. i.MX9) will
>> also reuse hab_sip_report_event. Could you send a fixup?
> 
> I fixed this part up here.

Thank you!

> 
> Sascha
>

Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |