mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn
@ 2024-01-11 15:42 Stefan Kerkmann
  2024-01-11 15:42 ` [PATCH 1/2] habv4: correct habv4 rom vector table Stefan Kerkmann
  2024-01-11 15:42 ` [PATCH 2/2] habv4: use hab rom implementation of report_event Stefan Kerkmann
  0 siblings, 2 replies; 7+ messages in thread
From: Stefan Kerkmann @ 2024-01-11 15:42 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX; +Cc: 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>
---
Stefan Kerkmann (2):
      habv4: correct habv4 rom vector table
      habv4: use hab rom implementation of report_event

 drivers/hab/habv4.c | 93 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 68 insertions(+), 25 deletions(-)
---
base-commit: 62748c3bf91d4f91f9f5c12c5fcd590c281d7b7f
change-id: 20240111-fix-habv4-event-report-aa1cc1fb7c95

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] habv4: correct habv4 rom vector table
  2024-01-11 15:42 [PATCH 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn Stefan Kerkmann
@ 2024-01-11 15:42 ` Stefan Kerkmann
  2024-01-11 15:57   ` Ahmad Fatoum
  2024-01-12  6:53   ` Marc Kleine-Budde
  2024-01-11 15:42 ` [PATCH 2/2] habv4: use hab rom implementation of report_event Stefan Kerkmann
  1 sibling, 2 replies; 7+ messages in thread
From: Stefan Kerkmann @ 2024-01-11 15:42 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX; +Cc: 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).

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

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index ed6d4db77c..dfa0207435 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -144,31 +144,41 @@ 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
+
+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,
+};
 
 static enum hab_status hab_sip_report_status(enum hab_config *config,
 					     enum habv4_state *state)
@@ -211,8 +221,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 +488,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 +535,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 +568,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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] habv4: use hab rom implementation of report_event
  2024-01-11 15:42 [PATCH 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn Stefan Kerkmann
  2024-01-11 15:42 ` [PATCH 1/2] habv4: correct habv4 rom vector table Stefan Kerkmann
@ 2024-01-11 15:42 ` Stefan Kerkmann
  1 sibling, 0 replies; 7+ messages in thread
From: Stefan Kerkmann @ 2024-01-11 15:42 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX; +Cc: 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 an imx8mm and imx8nm board
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 | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index dfa0207435..db5862a92a 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -180,6 +180,33 @@ enum hab_sip_cmd {
 	FSL_SIP_HAB_GET_VERSION = 0x07,
 };
 
+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)
 {
@@ -235,10 +262,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
@@ -292,9 +315,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())
+		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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] habv4: correct habv4 rom vector table
  2024-01-11 15:42 ` [PATCH 1/2] habv4: correct habv4 rom vector table Stefan Kerkmann
@ 2024-01-11 15:57   ` Ahmad Fatoum
  2024-01-12 15:26     ` Stefan Kerkmann
  2024-01-12  6:53   ` Marc Kleine-Budde
  1 sibling, 1 reply; 7+ messages in thread
From: Ahmad Fatoum @ 2024-01-11 15:57 UTC (permalink / raw)
  To: Stefan Kerkmann, Sascha Hauer, BAREBOX

Hello Stefan,

On 11.01.24 16:42, Stefan Kerkmann wrote:
> 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).
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>  drivers/hab/habv4.c | 50 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index ed6d4db77c..dfa0207435 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -144,31 +144,41 @@ 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);

While it only matter for forward declaration, perhaps, change this
to be a non-pointer like hab_loader_callback_fn above?

>  
> +/* 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);

Here you explicitly use a pointer to a function.

> +	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);

And here you rely on a function pointer being automatically derived.
While that's ok from a correctness point of view, for symmetry, it
would be better to stick to one type.

> +	uint32_t(* get_version)(void);

Nitpick: space after uint32_t and not before get_version.

> +	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

This is removed without replacement?

> -#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
> +
> +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,
> +};
>  
>  static enum hab_status hab_sip_report_status(enum hab_config *config,
>  					     enum habv4_state *state)
> @@ -211,8 +221,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 +488,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 +535,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 +568,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;
> 

-- 
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 |




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] habv4: correct habv4 rom vector table
  2024-01-11 15:42 ` [PATCH 1/2] habv4: correct habv4 rom vector table Stefan Kerkmann
  2024-01-11 15:57   ` Ahmad Fatoum
@ 2024-01-12  6:53   ` Marc Kleine-Budde
  1 sibling, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2024-01-12  6:53 UTC (permalink / raw)
  To: Stefan Kerkmann; +Cc: BAREBOX

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

On 11.01.2024 16:42:19, Stefan Kerkmann wrote:
> 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).
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

What's the size of a size_t on imx6? We don't want to break the 32 bit
platforms.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] habv4: correct habv4 rom vector table
  2024-01-11 15:57   ` Ahmad Fatoum
@ 2024-01-12 15:26     ` Stefan Kerkmann
  2024-01-12 16:04       ` Ahmad Fatoum
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Kerkmann @ 2024-01-12 15:26 UTC (permalink / raw)
  To: Ahmad Fatoum, Sascha Hauer, BAREBOX

Hello Ahmad,

On 11.01.24 16:57, Ahmad Fatoum wrote:
> Hello Stefan,
> 
>> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
>> index ed6d4db77c..dfa0207435 100644
>> --- a/drivers/hab/habv4.c
>> +++ b/drivers/hab/habv4.c
>> @@ -144,31 +144,41 @@ 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);
> 
> While it only matter for forward declaration, perhaps, change this
> to be a non-pointer like hab_loader_callback_fn above?

Done in v2.

>> +	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);
> 
> And here you rely on a function pointer being automatically derived.
> While that's ok from a correctness point of view, for symmetry, it
> would be better to stick to one type.

Done in v2.

>> +	uint32_t(* get_version)(void);
> 
> Nitpick: space after uint32_t and not before get_version.
>

Done in v2.

>> +	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
> 
> This is removed without replacement?
>

No it is still there, the diff is a bit misleading.

Regards,
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    |



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] habv4: correct habv4 rom vector table
  2024-01-12 15:26     ` Stefan Kerkmann
@ 2024-01-12 16:04       ` Ahmad Fatoum
  0 siblings, 0 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2024-01-12 16:04 UTC (permalink / raw)
  To: Stefan Kerkmann, Sascha Hauer, BAREBOX

On 12.01.24 16:26, Stefan Kerkmann wrote:
> Hello Ahmad,
> 
> On 11.01.24 16:57, Ahmad Fatoum wrote:
>> Hello Stefan,
>>
>>> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
>>> index ed6d4db77c..dfa0207435 100644
>>> --- a/drivers/hab/habv4.c
>>> +++ b/drivers/hab/habv4.c
>>> @@ -144,31 +144,41 @@ 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);
>>
>> While it only matter for forward declaration, perhaps, change this
>> to be a non-pointer like hab_loader_callback_fn above?
> 
> Done in v2.
> 
>>> +    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);
>>
>> And here you rely on a function pointer being automatically derived.
>> While that's ok from a correctness point of view, for symmetry, it
>> would be better to stick to one type.
> 
> Done in v2.
> 
>>> +    uint32_t(* get_version)(void);
>>
>> Nitpick: space after uint32_t and not before get_version.
>>
> 
> Done in v2.
> 
>>> +    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
>>
>> This is removed without replacement?
>>
> 
> No it is still there, the diff is a bit misleading.

Ah, its whitespace was changed..

> 
> Regards,
> Stefan
> 
> 
> 

-- 
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 |




^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-01-12 16:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-11 15:42 [PATCH 0/2] Use HABv4 report_event implementation for imx8mm and imx8mn Stefan Kerkmann
2024-01-11 15:42 ` [PATCH 1/2] habv4: correct habv4 rom vector table Stefan Kerkmann
2024-01-11 15:57   ` Ahmad Fatoum
2024-01-12 15:26     ` Stefan Kerkmann
2024-01-12 16:04       ` Ahmad Fatoum
2024-01-12  6:53   ` Marc Kleine-Budde
2024-01-11 15:42 ` [PATCH 2/2] habv4: use hab rom implementation of report_event Stefan Kerkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox