[PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Shanker Donthineni]

The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler
receiver interrupt") adds support for SGI interrupts in the FFA driver.
However, the validation for SGIs in the GICv3 is too strict, causing the
driver probe to fail.

This patch relaxes the SGI validation check, allowing callers to use SGIs
if the requested SGI number is greater than or equal to MAX_IPI, which
fixes the TFA driver probe failure.

This issue is observed on NVIDIA server platform with FFA-v1.1.
 [    7.918099] PTP clock support registered
 [    7.922110] EDAC MC: Ver: 3.0.0
 [    7.945063] ARM FF-A: Driver version 1.1
 [    7.949068] ARM FF-A: Firmware version 1.1 found
 [    7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request
 [    7.984237] ARM FF-A: Failed to create IRQ mapping!
 [    7.989220] ARM FF-A: Notification setup failed -61, not enabled
 [    8.000198] ARM FF-A: Failed to register driver sched callback -95
 [    8.011322] scmi_core: SCMI protocol bus registered

Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
---
 arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
 arch/arm64/kernel/smp.c             | 17 -----------------
 drivers/irqchip/irq-gic-v3.c        |  2 +-
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 9e96f024b2f19..ecf81df2915c7 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -188,5 +188,22 @@ static inline bool gic_has_relaxed_pmr_sync(void)
 	return cpus_have_cap(ARM64_HAS_GIC_PRIO_RELAXED_SYNC);
 }
 
+enum ipi_msg_type {
+	IPI_RESCHEDULE,
+	IPI_CALL_FUNC,
+	IPI_CPU_STOP,
+	IPI_CPU_CRASH_STOP,
+	IPI_TIMER,
+	IPI_IRQ_WORK,
+	NR_IPI,
+	/*
+	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
+	 * with trace_ipi_*
+	 */
+	IPI_CPU_BACKTRACE = NR_IPI,
+	IPI_KGDB_ROUNDUP,
+	MAX_IPI
+};
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARCH_GICV3_H */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 5e18fbcee9a20..373cd815d9a43 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -64,23 +64,6 @@ struct secondary_data secondary_data;
 /* Number of CPUs which aren't online, but looping in kernel text. */
 static int cpus_stuck_in_kernel;
 
-enum ipi_msg_type {
-	IPI_RESCHEDULE,
-	IPI_CALL_FUNC,
-	IPI_CPU_STOP,
-	IPI_CPU_CRASH_STOP,
-	IPI_TIMER,
-	IPI_IRQ_WORK,
-	NR_IPI,
-	/*
-	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
-	 * with trace_ipi_*
-	 */
-	IPI_CPU_BACKTRACE = NR_IPI,
-	IPI_KGDB_ROUNDUP,
-	MAX_IPI
-};
-
 static int ipi_irq_base __ro_after_init;
 static int nr_ipi __ro_after_init = NR_IPI;
 static struct irq_desc *ipi_desc[MAX_IPI] __ro_after_init;
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c19083bfb9432..0d2038d8cd311 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1655,7 +1655,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 		if(fwspec->param_count != 2)
 			return -EINVAL;
 
-		if (fwspec->param[0] < 16) {
+		if (fwspec->param[0] < MAX_IPI) {
 			pr_err(FW_BUG "Illegal GSI%d translation request\n",
 			       fwspec->param[0]);
 			return -EINVAL;
-- 
2.25.1

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Thomas Gleixner]

On Mon, Aug 12 2024 at 22:39, Shanker Donthineni wrote:
> The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler
> receiver interrupt") adds support for SGI interrupts in the FFA driver.
> However, the validation for SGIs in the GICv3 is too strict, causing the
> driver probe to fail.
>
> This patch relaxes the SGI validation check, allowing callers to use SGIs

# git grep "This patch" Documentation/process.

> if the requested SGI number is greater than or equal to MAX_IPI, which
> fixes the TFA driver probe failure.
>
> This issue is observed on NVIDIA server platform with FFA-v1.1.
>  [    7.918099] PTP clock support registered
>  [    7.922110] EDAC MC: Ver: 3.0.0
>  [    7.945063] ARM FF-A: Driver version 1.1
>  [    7.949068] ARM FF-A: Firmware version 1.1 found
>  [    7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request
>  [    7.984237] ARM FF-A: Failed to create IRQ mapping!
>  [    7.989220] ARM FF-A: Notification setup failed -61, not enabled
>  [    8.000198] ARM FF-A: Failed to register driver sched callback -95
>  [    8.011322] scmi_core: SCMI protocol bus registered

Please get rid of the time stamps and reduce it to the relevant
parts. Neither the time stamps nor PTP/EDAC/SCMI provide any useful
information.

Thanks,

        tglx

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Marc Zyngier]

On Tue, 13 Aug 2024 04:39:25 +0100,
Shanker Donthineni <sdonthineni@nvidia.com> wrote:
> 
> The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler
> receiver interrupt") adds support for SGI interrupts in the FFA driver.
> However, the validation for SGIs in the GICv3 is too strict, causing the
> driver probe to fail.

It probably is a good thing that I wasn't on Cc for this patch,
because I would have immediately NAK'd it. Sudeep, please consider
this a retrospective NAK!

> 
> This patch relaxes the SGI validation check, allowing callers to use SGIs
> if the requested SGI number is greater than or equal to MAX_IPI, which
> fixes the TFA driver probe failure.
> 
> This issue is observed on NVIDIA server platform with FFA-v1.1.
>  [    7.918099] PTP clock support registered
>  [    7.922110] EDAC MC: Ver: 3.0.0
>  [    7.945063] ARM FF-A: Driver version 1.1
>  [    7.949068] ARM FF-A: Firmware version 1.1 found
>  [    7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request
>  [    7.984237] ARM FF-A: Failed to create IRQ mapping!
>  [    7.989220] ARM FF-A: Notification setup failed -61, not enabled
>  [    8.000198] ARM FF-A: Failed to register driver sched callback -95
>  [    8.011322] scmi_core: SCMI protocol bus registered
> 
> Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
> ---
>  arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
>  arch/arm64/kernel/smp.c             | 17 -----------------
>  drivers/irqchip/irq-gic-v3.c        |  2 +-
>  3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 9e96f024b2f19..ecf81df2915c7 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -188,5 +188,22 @@ static inline bool gic_has_relaxed_pmr_sync(void)
>  	return cpus_have_cap(ARM64_HAS_GIC_PRIO_RELAXED_SYNC);
>  }
>  
> +enum ipi_msg_type {
> +	IPI_RESCHEDULE,
> +	IPI_CALL_FUNC,
> +	IPI_CPU_STOP,
> +	IPI_CPU_CRASH_STOP,
> +	IPI_TIMER,
> +	IPI_IRQ_WORK,
> +	NR_IPI,
> +	/*
> +	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
> +	 * with trace_ipi_*
> +	 */
> +	IPI_CPU_BACKTRACE = NR_IPI,
> +	IPI_KGDB_ROUNDUP,
> +	MAX_IPI
> +};
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_ARCH_GICV3_H */
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 5e18fbcee9a20..373cd815d9a43 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -64,23 +64,6 @@ struct secondary_data secondary_data;
>  /* Number of CPUs which aren't online, but looping in kernel text. */
>  static int cpus_stuck_in_kernel;
>  
> -enum ipi_msg_type {
> -	IPI_RESCHEDULE,
> -	IPI_CALL_FUNC,
> -	IPI_CPU_STOP,
> -	IPI_CPU_CRASH_STOP,
> -	IPI_TIMER,
> -	IPI_IRQ_WORK,
> -	NR_IPI,
> -	/*
> -	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
> -	 * with trace_ipi_*
> -	 */
> -	IPI_CPU_BACKTRACE = NR_IPI,
> -	IPI_KGDB_ROUNDUP,
> -	MAX_IPI
> -};
> -
>  static int ipi_irq_base __ro_after_init;
>  static int nr_ipi __ro_after_init = NR_IPI;
>  static struct irq_desc *ipi_desc[MAX_IPI] __ro_after_init;
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index c19083bfb9432..0d2038d8cd311 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1655,7 +1655,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
>  		if(fwspec->param_count != 2)
>  			return -EINVAL;
>  
> -		if (fwspec->param[0] < 16) {
> +		if (fwspec->param[0] < MAX_IPI) {
>  			pr_err(FW_BUG "Illegal GSI%d translation request\n",
>  			       fwspec->param[0]);
>  			return -EINVAL;

No. This is the wrong approach, and leads to inconsistent behaviour if
we ever change this MAX_IPI value. It also breaks 32 bit builds, and
makes things completely inconsistent between ACPI and DT.

I don't know how the FFA code was tested, because I cannot see how it
can work.

*IF* we are going to allow random SGIs being requested by random
drivers, we need to be able to do it properly. Not as a side hack like
this.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Sudeep Holla]

On Tue, Aug 13, 2024 at 09:58:34AM +0100, Marc Zyngier wrote:
> On Tue, 13 Aug 2024 04:39:25 +0100,
> Shanker Donthineni <sdonthineni@nvidia.com> wrote:
> > 
> > The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler
> > receiver interrupt") adds support for SGI interrupts in the FFA driver.
> > However, the validation for SGIs in the GICv3 is too strict, causing the
> > driver probe to fail.
> 
> It probably is a good thing that I wasn't on Cc for this patch,
> because I would have immediately NAK'd it. Sudeep, please consider
> this a retrospective NAK!
>

Sure, I am happy to work on any suggestions to replace it with better/cleaner
solution.

> > 
> > This patch relaxes the SGI validation check, allowing callers to use SGIs
> > if the requested SGI number is greater than or equal to MAX_IPI, which
> > fixes the TFA driver probe failure.
> > 
> > This issue is observed on NVIDIA server platform with FFA-v1.1.
> >  [    7.918099] PTP clock support registered
> >  [    7.922110] EDAC MC: Ver: 3.0.0
> >  [    7.945063] ARM FF-A: Driver version 1.1
> >  [    7.949068] ARM FF-A: Firmware version 1.1 found
> >  [    7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request
> >  [    7.984237] ARM FF-A: Failed to create IRQ mapping!
> >  [    7.989220] ARM FF-A: Notification setup failed -61, not enabled
> >  [    8.000198] ARM FF-A: Failed to register driver sched callback -95
> >  [    8.011322] scmi_core: SCMI protocol bus registered
> > 
> > Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
> > ---
> >  arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
> >  arch/arm64/kernel/smp.c             | 17 -----------------
> >  drivers/irqchip/irq-gic-v3.c        |  2 +-
> >  3 files changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> > index 9e96f024b2f19..ecf81df2915c7 100644
> > --- a/arch/arm64/include/asm/arch_gicv3.h
> > +++ b/arch/arm64/include/asm/arch_gicv3.h
> > @@ -188,5 +188,22 @@ static inline bool gic_has_relaxed_pmr_sync(void)
> >  	return cpus_have_cap(ARM64_HAS_GIC_PRIO_RELAXED_SYNC);
> >  }
> >  
> > +enum ipi_msg_type {
> > +	IPI_RESCHEDULE,
> > +	IPI_CALL_FUNC,
> > +	IPI_CPU_STOP,
> > +	IPI_CPU_CRASH_STOP,
> > +	IPI_TIMER,
> > +	IPI_IRQ_WORK,
> > +	NR_IPI,
> > +	/*
> > +	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
> > +	 * with trace_ipi_*
> > +	 */
> > +	IPI_CPU_BACKTRACE = NR_IPI,
> > +	IPI_KGDB_ROUNDUP,
> > +	MAX_IPI
> > +};
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* __ASM_ARCH_GICV3_H */
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index 5e18fbcee9a20..373cd815d9a43 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -64,23 +64,6 @@ struct secondary_data secondary_data;
> >  /* Number of CPUs which aren't online, but looping in kernel text. */
> >  static int cpus_stuck_in_kernel;
> >  
> > -enum ipi_msg_type {
> > -	IPI_RESCHEDULE,
> > -	IPI_CALL_FUNC,
> > -	IPI_CPU_STOP,
> > -	IPI_CPU_CRASH_STOP,
> > -	IPI_TIMER,
> > -	IPI_IRQ_WORK,
> > -	NR_IPI,
> > -	/*
> > -	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
> > -	 * with trace_ipi_*
> > -	 */
> > -	IPI_CPU_BACKTRACE = NR_IPI,
> > -	IPI_KGDB_ROUNDUP,
> > -	MAX_IPI
> > -};
> > -
> >  static int ipi_irq_base __ro_after_init;
> >  static int nr_ipi __ro_after_init = NR_IPI;
> >  static struct irq_desc *ipi_desc[MAX_IPI] __ro_after_init;
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index c19083bfb9432..0d2038d8cd311 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1655,7 +1655,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
> >  		if(fwspec->param_count != 2)
> >  			return -EINVAL;
> >
> > -		if (fwspec->param[0] < 16) {
> > +		if (fwspec->param[0] < MAX_IPI) {
> >  			pr_err(FW_BUG "Illegal GSI%d translation request\n",
> >  			       fwspec->param[0]);
> >  			return -EINVAL;
>
> No. This is the wrong approach, and leads to inconsistent behaviour if
> we ever change this MAX_IPI value. It also breaks 32 bit builds, and
> makes things completely inconsistent between ACPI and DT.
>
> I don't know how the FFA code was tested, because I cannot see how it
> can work.
>
> *IF* we are going to allow random SGIs being requested by random
> drivers, we need to be able to do it properly. Not as a side hack like
> this.

I am open for any ideas as FF-A spec authors/architects decided to allow
secure world to donate one of its SGI to the normal world for FF-A
notifications.

--
Regards,
Sudeep

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Marc Zyngier]

On Tue, 13 Aug 2024 11:33:47 +0100,
Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> On Tue, Aug 13, 2024 at 09:58:34AM +0100, Marc Zyngier wrote:
> > No. This is the wrong approach, and leads to inconsistent behaviour if
> > we ever change this MAX_IPI value. It also breaks 32 bit builds, and
> > makes things completely inconsistent between ACPI and DT.
> >
> > I don't know how the FFA code was tested, because I cannot see how it
> > can work.
> >
> > *IF* we are going to allow random SGIs being requested by random
> > drivers, we need to be able to do it properly. Not as a side hack like
> > this.
> 
> I am open for any ideas as FF-A spec authors/architects decided to allow
> secure world to donate one of its SGI to the normal world for FF-A
> notifications.

Let's first try to answer a simple question: how is that going to work
for interrupt architectures that do not have the concept of SGIs, but
rely on normal interrupts (similar to SPIs or LPIs) for their IPIs?

They don't have a global interrupt number per CPU for their IPIs, and
may not even have the concept of a shared numbering space between
security domains.

This makes the whole concept of "delegating" an interrupt number from
secure to non-secure a dead-end. Should we build a SW ecosystem on that?

The other thing is: if FFA is exposing interrupts to be signalled from
secure to non-secure, and that it insists on using SGIs, why isn't
that described in DT/ACPI, with a reservation mechanism that would
allow the GIC driver to reserve the corresponding SGI and not dish it
out as a normal mechanism?

Because this sort of thing

+       if (acpi_disabled) {
+               struct of_phandle_args oirq = {};
+               struct device_node *gic;
+
+               /* Only GICv3 supported currently with the device tree */
+               gic = of_find_compatible_node(NULL, NULL, "arm,gic-v3");
+               if (!gic)
+                       return -ENXIO;
+
+               oirq.np = gic;
+               oirq.args_count = 1;
+               oirq.args[0] = sr_intid;
+               irq = irq_create_of_mapping(&oirq);
+               of_node_put(gic);
+#ifdef CONFIG_ACPI
+       } else {
+               irq = acpi_register_gsi(NULL, sr_intid, ACPI_EDGE_SENSITIVE,
+                                       ACPI_ACTIVE_HIGH);
+#endif
+       }

is an absolute howler. It is abusing the arch-private interface, is at
the mercy of buggy EL3 returning stupid values, and may tramp over the
kernel's own IPI allocation.

All these problems need to be addressed.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Kai-Heng Feng]

Hi Sudeep,

On 2024/8/13 6:33 PM, Sudeep Holla wrote:
> On Tue, Aug 13, 2024 at 09:58:34AM +0100, Marc Zyngier wrote:
>> On Tue, 13 Aug 2024 04:39:25 +0100,
>> Shanker Donthineni <sdonthineni@nvidia.com> wrote:
>>>
>>> The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler
>>> receiver interrupt") adds support for SGI interrupts in the FFA driver.
>>> However, the validation for SGIs in the GICv3 is too strict, causing the
>>> driver probe to fail.
>>
>> It probably is a good thing that I wasn't on Cc for this patch,
>> because I would have immediately NAK'd it. Sudeep, please consider
>> this a retrospective NAK!
>>
> 
> Sure, I am happy to work on any suggestions to replace it with better/cleaner
> solution.
> 
>>>
>>> This patch relaxes the SGI validation check, allowing callers to use SGIs
>>> if the requested SGI number is greater than or equal to MAX_IPI, which
>>> fixes the TFA driver probe failure.
>>>
>>> This issue is observed on NVIDIA server platform with FFA-v1.1.
>>>   [    7.918099] PTP clock support registered
>>>   [    7.922110] EDAC MC: Ver: 3.0.0
>>>   [    7.945063] ARM FF-A: Driver version 1.1
>>>   [    7.949068] ARM FF-A: Firmware version 1.1 found
>>>   [    7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request
>>>   [    7.984237] ARM FF-A: Failed to create IRQ mapping!
>>>   [    7.989220] ARM FF-A: Notification setup failed -61, not enabled
>>>   [    8.000198] ARM FF-A: Failed to register driver sched callback -95
>>>   [    8.011322] scmi_core: SCMI protocol bus registered
>>>
>>> Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
>>> ---
>>>   arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
>>>   arch/arm64/kernel/smp.c             | 17 -----------------
>>>   drivers/irqchip/irq-gic-v3.c        |  2 +-
>>>   3 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
>>> index 9e96f024b2f19..ecf81df2915c7 100644
>>> --- a/arch/arm64/include/asm/arch_gicv3.h
>>> +++ b/arch/arm64/include/asm/arch_gicv3.h
>>> @@ -188,5 +188,22 @@ static inline bool gic_has_relaxed_pmr_sync(void)
>>>   	return cpus_have_cap(ARM64_HAS_GIC_PRIO_RELAXED_SYNC);
>>>   }
>>>   
>>> +enum ipi_msg_type {
>>> +	IPI_RESCHEDULE,
>>> +	IPI_CALL_FUNC,
>>> +	IPI_CPU_STOP,
>>> +	IPI_CPU_CRASH_STOP,
>>> +	IPI_TIMER,
>>> +	IPI_IRQ_WORK,
>>> +	NR_IPI,
>>> +	/*
>>> +	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
>>> +	 * with trace_ipi_*
>>> +	 */
>>> +	IPI_CPU_BACKTRACE = NR_IPI,
>>> +	IPI_KGDB_ROUNDUP,
>>> +	MAX_IPI
>>> +};
>>> +
>>>   #endif /* __ASSEMBLY__ */
>>>   #endif /* __ASM_ARCH_GICV3_H */
>>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>>> index 5e18fbcee9a20..373cd815d9a43 100644
>>> --- a/arch/arm64/kernel/smp.c
>>> +++ b/arch/arm64/kernel/smp.c
>>> @@ -64,23 +64,6 @@ struct secondary_data secondary_data;
>>>   /* Number of CPUs which aren't online, but looping in kernel text. */
>>>   static int cpus_stuck_in_kernel;
>>>   
>>> -enum ipi_msg_type {
>>> -	IPI_RESCHEDULE,
>>> -	IPI_CALL_FUNC,
>>> -	IPI_CPU_STOP,
>>> -	IPI_CPU_CRASH_STOP,
>>> -	IPI_TIMER,
>>> -	IPI_IRQ_WORK,
>>> -	NR_IPI,
>>> -	/*
>>> -	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
>>> -	 * with trace_ipi_*
>>> -	 */
>>> -	IPI_CPU_BACKTRACE = NR_IPI,
>>> -	IPI_KGDB_ROUNDUP,
>>> -	MAX_IPI
>>> -};
>>> -
>>>   static int ipi_irq_base __ro_after_init;
>>>   static int nr_ipi __ro_after_init = NR_IPI;
>>>   static struct irq_desc *ipi_desc[MAX_IPI] __ro_after_init;
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index c19083bfb9432..0d2038d8cd311 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -1655,7 +1655,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
>>>   		if(fwspec->param_count != 2)
>>>   			return -EINVAL;
>>>
>>> -		if (fwspec->param[0] < 16) {
>>> +		if (fwspec->param[0] < MAX_IPI) {
>>>   			pr_err(FW_BUG "Illegal GSI%d translation request\n",
>>>   			       fwspec->param[0]);
>>>   			return -EINVAL;
>>
>> No. This is the wrong approach, and leads to inconsistent behaviour if
>> we ever change this MAX_IPI value. It also breaks 32 bit builds, and
>> makes things completely inconsistent between ACPI and DT.
>>
>> I don't know how the FFA code was tested, because I cannot see how it
>> can work.
>>
>> *IF* we are going to allow random SGIs being requested by random
>> drivers, we need to be able to do it properly. Not as a side hack like
>> this.
> 
> I am open for any ideas as FF-A spec authors/architects decided to allow
> secure world to donate one of its SGI to the normal world for FF-A
> notifications.

Is there any progression on this issue? Do you think it's reasonable to revert 
commit 897e9e60c016 as a temporary measure?

Kai-Heng

> 
> --
> Regards,
> Sudeep
> 

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Sudeep Holla]

On Mon, Dec 16, 2024 at 10:25:29AM +0800, Kai-Heng Feng wrote:
> Hi Sudeep,
> 
> On 2024/8/13 6:33 PM, Sudeep Holla wrote:
> > On Tue, Aug 13, 2024 at 09:58:34AM +0100, Marc Zyngier wrote:
> > > On Tue, 13 Aug 2024 04:39:25 +0100,
> > > Shanker Donthineni <sdonthineni@nvidia.com> wrote:
> > > > 
> > > > The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler
> > > > receiver interrupt") adds support for SGI interrupts in the FFA driver.
> > > > However, the validation for SGIs in the GICv3 is too strict, causing the
> > > > driver probe to fail.
> > > 
> > > It probably is a good thing that I wasn't on Cc for this patch,
> > > because I would have immediately NAK'd it. Sudeep, please consider
> > > this a retrospective NAK!
> > > 
> > 
> > Sure, I am happy to work on any suggestions to replace it with better/cleaner
> > solution.
> > 
> > > > 
> > > > This patch relaxes the SGI validation check, allowing callers to use SGIs
> > > > if the requested SGI number is greater than or equal to MAX_IPI, which
> > > > fixes the TFA driver probe failure.
> > > > 
> > > > This issue is observed on NVIDIA server platform with FFA-v1.1.
> > > >   [    7.918099] PTP clock support registered
> > > >   [    7.922110] EDAC MC: Ver: 3.0.0
> > > >   [    7.945063] ARM FF-A: Driver version 1.1
> > > >   [    7.949068] ARM FF-A: Firmware version 1.1 found
> > > >   [    7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request
> > > >   [    7.984237] ARM FF-A: Failed to create IRQ mapping!
> > > >   [    7.989220] ARM FF-A: Notification setup failed -61, not enabled
> > > >   [    8.000198] ARM FF-A: Failed to register driver sched callback -95
> > > >   [    8.011322] scmi_core: SCMI protocol bus registered
> > > > 
> > > > Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
> > > > ---
> > > >   arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
> > > >   arch/arm64/kernel/smp.c             | 17 -----------------
> > > >   drivers/irqchip/irq-gic-v3.c        |  2 +-
> > > >   3 files changed, 18 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> > > > index 9e96f024b2f19..ecf81df2915c7 100644
> > > > --- a/arch/arm64/include/asm/arch_gicv3.h
> > > > +++ b/arch/arm64/include/asm/arch_gicv3.h
> > > > @@ -188,5 +188,22 @@ static inline bool gic_has_relaxed_pmr_sync(void)
> > > >   	return cpus_have_cap(ARM64_HAS_GIC_PRIO_RELAXED_SYNC);
> > > >   }
> > > > +enum ipi_msg_type {
> > > > +	IPI_RESCHEDULE,
> > > > +	IPI_CALL_FUNC,
> > > > +	IPI_CPU_STOP,
> > > > +	IPI_CPU_CRASH_STOP,
> > > > +	IPI_TIMER,
> > > > +	IPI_IRQ_WORK,
> > > > +	NR_IPI,
> > > > +	/*
> > > > +	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
> > > > +	 * with trace_ipi_*
> > > > +	 */
> > > > +	IPI_CPU_BACKTRACE = NR_IPI,
> > > > +	IPI_KGDB_ROUNDUP,
> > > > +	MAX_IPI
> > > > +};
> > > > +
> > > >   #endif /* __ASSEMBLY__ */
> > > >   #endif /* __ASM_ARCH_GICV3_H */
> > > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > > > index 5e18fbcee9a20..373cd815d9a43 100644
> > > > --- a/arch/arm64/kernel/smp.c
> > > > +++ b/arch/arm64/kernel/smp.c
> > > > @@ -64,23 +64,6 @@ struct secondary_data secondary_data;
> > > >   /* Number of CPUs which aren't online, but looping in kernel text. */
> > > >   static int cpus_stuck_in_kernel;
> > > > -enum ipi_msg_type {
> > > > -	IPI_RESCHEDULE,
> > > > -	IPI_CALL_FUNC,
> > > > -	IPI_CPU_STOP,
> > > > -	IPI_CPU_CRASH_STOP,
> > > > -	IPI_TIMER,
> > > > -	IPI_IRQ_WORK,
> > > > -	NR_IPI,
> > > > -	/*
> > > > -	 * Any enum >= NR_IPI and < MAX_IPI is special and not tracable
> > > > -	 * with trace_ipi_*
> > > > -	 */
> > > > -	IPI_CPU_BACKTRACE = NR_IPI,
> > > > -	IPI_KGDB_ROUNDUP,
> > > > -	MAX_IPI
> > > > -};
> > > > -
> > > >   static int ipi_irq_base __ro_after_init;
> > > >   static int nr_ipi __ro_after_init = NR_IPI;
> > > >   static struct irq_desc *ipi_desc[MAX_IPI] __ro_after_init;
> > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > index c19083bfb9432..0d2038d8cd311 100644
> > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > @@ -1655,7 +1655,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
> > > >   		if(fwspec->param_count != 2)
> > > >   			return -EINVAL;
> > > > 
> > > > -		if (fwspec->param[0] < 16) {
> > > > +		if (fwspec->param[0] < MAX_IPI) {
> > > >   			pr_err(FW_BUG "Illegal GSI%d translation request\n",
> > > >   			       fwspec->param[0]);
> > > >   			return -EINVAL;
> > > 
> > > No. This is the wrong approach, and leads to inconsistent behaviour if
> > > we ever change this MAX_IPI value. It also breaks 32 bit builds, and
> > > makes things completely inconsistent between ACPI and DT.
> > > 
> > > I don't know how the FFA code was tested, because I cannot see how it
> > > can work.
> > > 
> > > *IF* we are going to allow random SGIs being requested by random
> > > drivers, we need to be able to do it properly. Not as a side hack like
> > > this.
> > 
> > I am open for any ideas as FF-A spec authors/architects decided to allow
> > secure world to donate one of its SGI to the normal world for FF-A
> > notifications.
> 
> Is there any progression on this issue? Do you think it's reasonable to
> revert commit 897e9e60c016 as a temporary measure?
> 

I haven't started on this yet, but I plan to do that in a week or so.
Revert may not be trivial at this point. We can disable it until it is
fixed properly instead of all possible conflicts with reverts.

-- 
Regards,
Sudeep

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[kernel test robot]

Hi Shanker,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on tip/irq/core soc/for-next linus/master v6.11-rc3 next-20240814]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shanker-Donthineni/irqchip-gic-v3-Allow-unused-SGIs-for-drivers-modules/20240814-221122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20240813033925.925947-1-sdonthineni%40nvidia.com
patch subject: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules
config: arm-randconfig-004-20240815 (https://download.01.org/0day-ci/archive/20240815/202408151138.JtTxkiQ6-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/202408151138.JtTxkiQ6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408151138.JtTxkiQ6-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/irqchip/irq-gic-v3.c: In function 'gic_irq_domain_translate':
>> drivers/irqchip/irq-gic-v3.c:1658:40: error: 'MAX_IPI' undeclared (first use in this function); did you mean 'MAX_INPUT'?
    1658 |                 if (fwspec->param[0] < MAX_IPI) {
         |                                        ^~~~~~~
         |                                        MAX_INPUT
   drivers/irqchip/irq-gic-v3.c:1658:40: note: each undeclared identifier is reported only once for each function it appears in


vim +1658 drivers/irqchip/irq-gic-v3.c

  1600	
  1601	static int gic_irq_domain_translate(struct irq_domain *d,
  1602					    struct irq_fwspec *fwspec,
  1603					    unsigned long *hwirq,
  1604					    unsigned int *type)
  1605	{
  1606		if (fwspec->param_count == 1 && fwspec->param[0] < 16) {
  1607			*hwirq = fwspec->param[0];
  1608			*type = IRQ_TYPE_EDGE_RISING;
  1609			return 0;
  1610		}
  1611	
  1612		if (is_of_node(fwspec->fwnode)) {
  1613			if (fwspec->param_count < 3)
  1614				return -EINVAL;
  1615	
  1616			switch (fwspec->param[0]) {
  1617			case 0:			/* SPI */
  1618				*hwirq = fwspec->param[1] + 32;
  1619				break;
  1620			case 1:			/* PPI */
  1621				*hwirq = fwspec->param[1] + 16;
  1622				break;
  1623			case 2:			/* ESPI */
  1624				*hwirq = fwspec->param[1] + ESPI_BASE_INTID;
  1625				break;
  1626			case 3:			/* EPPI */
  1627				*hwirq = fwspec->param[1] + EPPI_BASE_INTID;
  1628				break;
  1629			case GIC_IRQ_TYPE_LPI:	/* LPI */
  1630				*hwirq = fwspec->param[1];
  1631				break;
  1632			case GIC_IRQ_TYPE_PARTITION:
  1633				*hwirq = fwspec->param[1];
  1634				if (fwspec->param[1] >= 16)
  1635					*hwirq += EPPI_BASE_INTID - 16;
  1636				else
  1637					*hwirq += 16;
  1638				break;
  1639			default:
  1640				return -EINVAL;
  1641			}
  1642	
  1643			*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
  1644	
  1645			/*
  1646			 * Make it clear that broken DTs are... broken.
  1647			 * Partitioned PPIs are an unfortunate exception.
  1648			 */
  1649			WARN_ON(*type == IRQ_TYPE_NONE &&
  1650				fwspec->param[0] != GIC_IRQ_TYPE_PARTITION);
  1651			return 0;
  1652		}
  1653	
  1654		if (is_fwnode_irqchip(fwspec->fwnode)) {
  1655			if(fwspec->param_count != 2)
  1656				return -EINVAL;
  1657	
> 1658			if (fwspec->param[0] < MAX_IPI) {
  1659				pr_err(FW_BUG "Illegal GSI%d translation request\n",
  1660				       fwspec->param[0]);
  1661				return -EINVAL;
  1662			}
  1663	
  1664			*hwirq = fwspec->param[0];
  1665			*type = fwspec->param[1];
  1666	
  1667			WARN_ON(*type == IRQ_TYPE_NONE);
  1668			return 0;
  1669		}
  1670	
  1671		return -EINVAL;
  1672	}
  1673	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[kernel test robot]

Hi Shanker,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on tip/irq/core soc/for-next linus/master v6.11-rc3 next-20240814]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shanker-Donthineni/irqchip-gic-v3-Allow-unused-SGIs-for-drivers-modules/20240814-221122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20240813033925.925947-1-sdonthineni%40nvidia.com
patch subject: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240815/202408151208.vuvL1AtV-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/202408151208.vuvL1AtV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408151208.vuvL1AtV-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/irqchip/irq-gic-v3.c:1658:26: error: use of undeclared identifier 'MAX_IPI'
                   if (fwspec->param[0] < MAX_IPI) {
                                          ^
   1 error generated.


vim +/MAX_IPI +1658 drivers/irqchip/irq-gic-v3.c

  1600	
  1601	static int gic_irq_domain_translate(struct irq_domain *d,
  1602					    struct irq_fwspec *fwspec,
  1603					    unsigned long *hwirq,
  1604					    unsigned int *type)
  1605	{
  1606		if (fwspec->param_count == 1 && fwspec->param[0] < 16) {
  1607			*hwirq = fwspec->param[0];
  1608			*type = IRQ_TYPE_EDGE_RISING;
  1609			return 0;
  1610		}
  1611	
  1612		if (is_of_node(fwspec->fwnode)) {
  1613			if (fwspec->param_count < 3)
  1614				return -EINVAL;
  1615	
  1616			switch (fwspec->param[0]) {
  1617			case 0:			/* SPI */
  1618				*hwirq = fwspec->param[1] + 32;
  1619				break;
  1620			case 1:			/* PPI */
  1621				*hwirq = fwspec->param[1] + 16;
  1622				break;
  1623			case 2:			/* ESPI */
  1624				*hwirq = fwspec->param[1] + ESPI_BASE_INTID;
  1625				break;
  1626			case 3:			/* EPPI */
  1627				*hwirq = fwspec->param[1] + EPPI_BASE_INTID;
  1628				break;
  1629			case GIC_IRQ_TYPE_LPI:	/* LPI */
  1630				*hwirq = fwspec->param[1];
  1631				break;
  1632			case GIC_IRQ_TYPE_PARTITION:
  1633				*hwirq = fwspec->param[1];
  1634				if (fwspec->param[1] >= 16)
  1635					*hwirq += EPPI_BASE_INTID - 16;
  1636				else
  1637					*hwirq += 16;
  1638				break;
  1639			default:
  1640				return -EINVAL;
  1641			}
  1642	
  1643			*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
  1644	
  1645			/*
  1646			 * Make it clear that broken DTs are... broken.
  1647			 * Partitioned PPIs are an unfortunate exception.
  1648			 */
  1649			WARN_ON(*type == IRQ_TYPE_NONE &&
  1650				fwspec->param[0] != GIC_IRQ_TYPE_PARTITION);
  1651			return 0;
  1652		}
  1653	
  1654		if (is_fwnode_irqchip(fwspec->fwnode)) {
  1655			if(fwspec->param_count != 2)
  1656				return -EINVAL;
  1657	
> 1658			if (fwspec->param[0] < MAX_IPI) {
  1659				pr_err(FW_BUG "Illegal GSI%d translation request\n",
  1660				       fwspec->param[0]);
  1661				return -EINVAL;
  1662			}
  1663	
  1664			*hwirq = fwspec->param[0];
  1665			*type = fwspec->param[1];
  1666	
  1667			WARN_ON(*type == IRQ_TYPE_NONE);
  1668			return 0;
  1669		}
  1670	
  1671		return -EINVAL;
  1672	}
  1673	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki