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

[Stefan Kerkmann <s.kerkmann@pengutronix.de>]

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    |