Function _local_event_enable is used for private sdei event
registeration called by sdei_event_register. We should pass
sdei_api_event_register right flag and mpidr parameters, otherwise atf
may trigger assert errors.
Signed-off-by: Liguang Zhang <[email protected]>
---
drivers/firmware/arm_sdei.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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);
sdei_cross_call_return(arg, err);
}
--
2.19.1.6.gb485710b
Hi James,
Gentle ping! Any comments on this patch?
?? 2021/9/10 12:01, Liguang Zhang д??:
> Function _local_event_enable is used for private sdei event
> registeration called by sdei_event_register. We should pass
> sdei_api_event_register right flag and mpidr parameters, otherwise atf
> may trigger assert errors.
>
> Signed-off-by: Liguang Zhang <[email protected]>
> ---
> drivers/firmware/arm_sdei.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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);
>
> sdei_cross_call_return(arg, err);
> }
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.
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.)
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);
> }
>
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);
>> }
>>
Hi Liguang,
On 11/10/2021 06:40, 乱石 wrote:
> 在 2021/10/9 1:39, James Morse 写道:
>> 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.
> For a private event, we route SDEI_EVENT_REGISTER_RM_PE and mpidr parameters may be more
> rationable.
You are making this call from Linux?
This isn't valid for private events. Private events are private to the CPU - they can only
be reset, register and taken on that CPU. The specification for SDEI_EVENT_ROUTING_SET has
this:
| This call is used to change the routing information of a shared event.
To borrow the GIC's terms: Private events are like PPI, Shared events are like SPI.
>> 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
Is that an SPI? If so, you should really be generating a shared event.
> for hardlockup detection, in os
> panic ,we mask sdei event, then trigger the assert
> if (se->reg_flags == SDEI_REGF_RM_PE)
>
> assert(se->affinity == my_mpidr);
I'm not sure where this code in TFA is, but RM_PE for a private event is going to hit this
on all but one CPU. You shouldn't be able to set RM_PE for a private event.
I assume this is the TFA side of the problem from your colleague:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/11393
Does the problem occur with this TFA patch applied, and without any attempt to mess with
the routing of per-cpu/private events?
Thanks,
James
Hi James,
在 2021/10/19 1:32, James Morse 写道:
> Hi Liguang,
>
> On 11/10/2021 06:40, 乱石 wrote:
>> 在 2021/10/9 1:39, James Morse 写道:
>>> 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.
>
>> For a private event, we route SDEI_EVENT_REGISTER_RM_PE and mpidr parameters may be more
>> rationable.
> You are making this call from Linux?
>
> This isn't valid for private events. Private events are private to the CPU - they can only
> be reset, register and taken on that CPU. The specification for SDEI_EVENT_ROUTING_SET has
> this:
> | This call is used to change the routing information of a shared event.
>
> To borrow the GIC's terms: Private events are like PPI, Shared events are like SPI.
>
>
>>> 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
> Is that an SPI? If so, you should really be generating a shared event.
It's an PPI, secured arch timer used for hardlockup detection.
>
>
>> for hardlockup detection, in os
>> panic ,we mask sdei event, then trigger the assert
>> if (se->reg_flags == SDEI_REGF_RM_PE)
>>
>> assert(se->affinity == my_mpidr);
>
> I'm not sure where this code in TFA is, but RM_PE for a private event is going to hit this
> on all but one CPU. You shouldn't be able to set RM_PE for a private event.
>
>
> I assume this is the TFA side of the problem from your colleague:
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/11393
>
>
> Does the problem occur with this TFA patch applied, and without any attempt to mess with
> the routing of per-cpu/private events?
Thanks for your reply. With the patch applied, the problem resolved.
Thanks,
Liguang
>
>
> Thanks,
>
> James