Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters

[乱石]

Hi James,

在 2021/10/9 1:39, James Morse 写道:
> Hello!
>
> (sorry for the delayed response)
>
> On 10/09/2021 05:01, Liguang Zhang wrote:
> > Function _local_event_enable is used for private sdei event
> > registeration called by sdei_event_register. We should pass
> (registration)
>
> > sdei_api_event_register right flag and mpidr parameters, otherwise atf
> > may trigger assert errors.
> > diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> > index a7e762c352f9..0736752dadde 100644
> > --- a/drivers/firmware/arm_sdei.c
> > +++ b/drivers/firmware/arm_sdei.c
> > @@ -558,14 +558,16 @@ static int sdei_api_event_register(u32 event_num, unsigned long entry_point,
> >   static void _local_event_register(void *data)
> >   {
> >   	int err;
> > +	u64 mpidr;
> >   	struct sdei_registered_event *reg;
> >   	struct sdei_crosscall_args *arg = data;
> >   
> >   	WARN_ON(preemptible());
> >   
> > +	mpidr = read_cpuid_mpidr();
> >   	reg = per_cpu_ptr(arg->event->private_registered, smp_processor_id());
> >   	err = sdei_api_event_register(arg->event->event_num, sdei_entry_point,
> > -				      reg, 0, 0);
> > +				      reg, SDEI_EVENT_REGISTER_RM_PE, mpidr);
> Hmmm, this looks like a bug in TFA.
>
> 5.1.2.2 "Parameters" of DEN 0054B has:
> | Routing mode is valid only for a shared event. For a private event, the routing mode is
> | ignored.
>
> Worse, the mpidr field has:
> | Currently the format is defined only when the selected routing mode is RM_PE.

or a private event, we route SDEI_EVENT_REGISTER_RM_PE and mpidr 
parameters may be more rationable.


> Over in trusted firmware land:
>
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/services/std_svc/sdei/sdei_main.c?h=v2.5#n361
>
> | static int64_t sdei_event_register(int ev_num,
> | 				uint64_t ep,
> | 				uint64_t arg,
> | 				uint64_t flags,
> |				uint64_t mpidr)
> | {
>
> |	/* Private events always target the PE */
> |	if (is_event_private(map))
> |		flags = SDEI_REGF_RM_PE;
>
> It looks like this re-uses the 'caller specified the routing' code, but didn't update the
> mpidr.
>
>
> You mention TFA takes an assert failure, I assume that brings the machine down.
> (Presumably you don't have a CPU with an affinity of zero.)

Yes, that brings the machine down. In opensource ATF, CPU with an 
affinity of zero.

The problem backaround:

we use local secure arch timer as sdei watchdog timer for hardlockup 
detection, in  os panic flow, we mask sdei event, then trigger the assert


if (se->reg_flags == SDEI_REGF_RM_PE)

    assert(se->affinity == my_mpidr);


Thanks,

Liguang

> Does this mean no-one relies on this, and we can fix the firmware?
> (I'll go report this to the relevant folk)
>
>
> Thanks!
>
> James
>
>
> >   
> >   	sdei_cross_call_return(arg, err);
> >   }