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

[乱石 <zhangliguang@linux.alibaba.com>]

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);
>>   }
>>