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