Messages in this thread | | | Subject | Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters | From | 乱石 <> | Date | Mon, 11 Oct 2021 18:37:01 +0800 |
| |
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); >> } >>
| |