[PATCH] firmware: arm_sdei: pass sdei_api_event_register right parameters

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 <redacted>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi James,

Gentle ping! Any comments on this patch?


在 2021/9/10 12:01, Liguang Zhang 写道:

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-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:

quoted

Function _local_event_enable is used for private sdei event
registeration called by sdei_event_register. We should pass

(registration)

quoted

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

quoted

  
  	sdei_cross_call_return(arg, err);
  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel