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

[乱石]

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