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

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |