* [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters @ 2021-09-10 4:01 Liguang Zhang 2021-09-24 13:34 ` 乱石 2021-10-08 17:39 ` James Morse 0 siblings, 2 replies; 6+ messages in thread From: Liguang Zhang @ 2021-09-10 4:01 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel, 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 <zhangliguang@linux.alibaba.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters 2021-09-10 4:01 [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters Liguang Zhang @ 2021-09-24 13:34 ` 乱石 2021-10-08 17:39 ` James Morse 1 sibling, 0 replies; 6+ messages in thread From: 乱石 @ 2021-09-24 13:34 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel 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 <zhangliguang@linux.alibaba.com> > --- > 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); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters 2021-09-10 4:01 [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters Liguang Zhang 2021-09-24 13:34 ` 乱石 @ 2021-10-08 17:39 ` James Morse 2021-10-11 10:37 ` 乱石 [not found] ` <b93cf74a-ec1a-dcfc-990b-d3dbc4b55c3d@linux.alibaba.com> 1 sibling, 2 replies; 6+ messages in thread From: James Morse @ 2021-10-08 17:39 UTC (permalink / raw) To: Liguang Zhang; +Cc: linux-arm-kernel, linux-kernel 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); > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters 2021-10-08 17:39 ` James Morse @ 2021-10-11 10:37 ` 乱石 [not found] ` <b93cf74a-ec1a-dcfc-990b-d3dbc4b55c3d@linux.alibaba.com> 1 sibling, 0 replies; 6+ messages in thread From: 乱石 @ 2021-10-11 10:37 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel 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); >> } >> ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <b93cf74a-ec1a-dcfc-990b-d3dbc4b55c3d@linux.alibaba.com>]
* Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters [not found] ` <b93cf74a-ec1a-dcfc-990b-d3dbc4b55c3d@linux.alibaba.com> @ 2021-10-18 17:32 ` James Morse 2021-10-19 3:35 ` 乱石 0 siblings, 1 reply; 6+ messages in thread From: James Morse @ 2021-10-18 17:32 UTC (permalink / raw) To: 乱石; +Cc: linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters 2021-10-18 17:32 ` James Morse @ 2021-10-19 3:35 ` 乱石 0 siblings, 0 replies; 6+ messages in thread From: 乱石 @ 2021-10-19 3:35 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-19 3:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 4:01 [PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters Liguang Zhang
2021-09-24 13:34 ` 乱石
2021-10-08 17:39 ` James Morse
2021-10-11 10:37 ` 乱石
[not found] ` <b93cf74a-ec1a-dcfc-990b-d3dbc4b55c3d@linux.alibaba.com>
2021-10-18 17:32 ` James Morse
2021-10-19 3:35 ` 乱石
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).